| Message ID | 20250221-rmv_return-v1-1-cc8dff275827@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Remove weird and needless 'return' for void APIs | expand |
On 2025/2/22 04:01, Peter Zijlstra wrote: >> */ >> static inline void tlb_remove_page(struct mmu_gather *tlb, struct page *page) >> { >> - return tlb_remove_page_size(tlb, page, PAGE_SIZE); >> + tlb_remove_page_size(tlb, page, PAGE_SIZE); >> } > So I don't mind removing it, but note that that return enforces > tlb_remove_page_size() has void return type. > tlb_remove_page_size() is void function already. (^^) > It might not be your preferred coding style, but it is not completely > pointless. based on below C spec such as C17 description. i guess language C does not like this usage "return void function in void function"; C spec such as C17 have this description about return statement: 6.8.6.4: A return statement with an expression shall not appear in a function whose return type is void. A return statement without an expression shall only appear in a function whose return type is void.
Zijun Hu <zijun_hu@icloud.com> wrote: > >> static inline void tlb_remove_page(struct mmu_gather *tlb, struct page *page) > >> { > >> - return tlb_remove_page_size(tlb, page, PAGE_SIZE); > >> + tlb_remove_page_size(tlb, page, PAGE_SIZE); > >> } > > So I don't mind removing it, but note that that return enforces > > tlb_remove_page_size() has void return type. > > > > tlb_remove_page_size() is void function already. (^^) That may be true... for now. But if that is changed in the future, then you will get an error indicating something you need to go and look at... so in that regard, it's *better* to do this ;-) David
On 2/24/25 15:45, Zijun Hu wrote: > On 2025/2/24 21:23, Peter Zijlstra wrote: >> On Sat, Feb 22, 2025 at 07:00:28PM +0800, Zijun Hu wrote: >>> On 2025/2/22 04:01, Peter Zijlstra wrote: >>>>> */ >>>>> static inline void tlb_remove_page(struct mmu_gather *tlb, struct page *page) >>>>> { >>>>> - return tlb_remove_page_size(tlb, page, PAGE_SIZE); >>>>> + tlb_remove_page_size(tlb, page, PAGE_SIZE); >>>>> } >>>> So I don't mind removing it, but note that that return enforces >>>> tlb_remove_page_size() has void return type. >>>> >>> >>> tlb_remove_page_size() is void function already. (^^) >> >> Yes, but if you were to change that, the above return would complain. >> >>>> It might not be your preferred coding style, but it is not completely >>>> pointless. >>> >>> based on below C spec such as C17 description. i guess language C does >>> not like this usage "return void function in void function"; >> >> This is GNU extension IIRC. Note kernel uses GNU11, not C11 > > any link to share about GNU11's description for this aspect ? (^^) this is new for C17 or was there for long time? even if this is an extension, it is very nice for generating locked wrappers, so you don't have to handle void case specially void foo_bar(...) { lockdep_assert_held(&a_lock); /// ... } // generated void foo_bar_lock(...) { scoped_guard(mutex, &a_lock) return foo_bar(...); } etc
On 2025/2/26 01:27, Przemek Kitszel wrote: >>>>> It might not be your preferred coding style, but it is not completely >>>>> pointless. >>>> >>>> based on below C spec such as C17 description. i guess language C does >>>> not like this usage "return void function in void function"; >>> >>> This is GNU extension IIRC. Note kernel uses GNU11, not C11 >> >> any link to share about GNU11's description for this aspect ? (^^) > this is new for C17 or was there for long time? > Standard C spec has that description for long time. Standard C11 spec also has that description. > even if this is an extension, it is very nice for generating locked > wrappers, so you don't have to handle void case specially > > void foo_bar(...) > { > lockdep_assert_held(&a_lock); > /// ... > } > > // generated > void foo_bar_lock(...) > { > scoped_guard(mutex, &a_lock) > return foo_bar(...); above is able to be written as below: scoped_guard(mutex, &a_lock) { foo_bar(...); return; } > } i will list my reasons why this usage "return void function in void function" is not good in cover letter [00/18] of this series.
On 2025/2/25 23:16, David Howells wrote: > Zijun Hu <zijun_hu@icloud.com> wrote: > >>>> static inline void tlb_remove_page(struct mmu_gather *tlb, struct page *page) >>>> { >>>> - return tlb_remove_page_size(tlb, page, PAGE_SIZE); >>>> + tlb_remove_page_size(tlb, page, PAGE_SIZE); >>>> } >>> So I don't mind removing it, but note that that return enforces >>> tlb_remove_page_size() has void return type. >>> >> >> tlb_remove_page_size() is void function already. (^^) > > That may be true... for now. But if that is changed in the future, then you > will get an error indicating something you need to go and look at... so in > that regard, it's *better* to do this ;-) > i understand your point. if the callee tlb_remove_page_size() is in the same module with the caller tlb_remove_page. it is meaningless to watch the callee's return type. otherwise, provided the callee is a API which is provided by other module author. once the author changes the API's return type, he/she must take effort to cleanup this weird and lots of usages, that is not nice for API provider. this is a common issue. i will list my reasons why this usage is not good in cover letter of this series > David >
diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h index e402aef79c93..812110813b84 100644 --- a/include/asm-generic/tlb.h +++ b/include/asm-generic/tlb.h @@ -501,7 +501,7 @@ static __always_inline bool __tlb_remove_page(struct mmu_gather *tlb, */ static inline void tlb_remove_page(struct mmu_gather *tlb, struct page *page) { - return tlb_remove_page_size(tlb, page, PAGE_SIZE); + tlb_remove_page_size(tlb, page, PAGE_SIZE); } static inline void tlb_remove_ptdesc(struct mmu_gather *tlb, void *pt)
Remove needless 'return' in void API tlb_remove_page() since both the API and tlb_remove_page_size() are void functions. Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> --- include/asm-generic/tlb.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)