On 2018/12/19 3:51, Andreas Dilger Wrote:
> On Dec 18, 2018, at 5:00 AM, zhangyi (F) <yi.zhang@xxxxxxxxxx> wrote:
>>
>> Create separate predicate functions to test/set/clear/test_and_set
>> bb_state flags in ext4_group_info like features testing, and then
>> replace all old macros and the places where we use
>> EXT4_GROUP_INFO_XXX_BIT directly.
>>
>> Signed-off-by: zhangyi (F) <yi.zhang@xxxxxxxxxx>
>> ---
>> +#define EXT4_MB_GROUP_STATE_FUNCS(name, statename) \
>> +static inline int ext4_mb_grp_##name(struct ext4_group_info *grp) \
>> +{ \
>> + return test_bit(EXT4_GROUP_INFO_##statename##_BIT, \
>> + &(grp->bb_state)); \
>> +} \
>> +static inline void ext4_mb_grp_set_##name(struct ext4_group_info *grp) \
>> +{ \
>> + set_bit(EXT4_GROUP_INFO_##statename##_BIT, &(grp->bb_state)); \
>> +} \
>> +static inline void ext4_mb_grp_clear_##name(struct ext4_group_info *grp)\
>> +{ \
>> + clear_bit(EXT4_GROUP_INFO_##statename##_BIT, &(grp->bb_state)); \
>> +} \
>> +static inline int ext4_mb_grp_test_and_set_##name(struct ext4_group_info *grp) \
>> +{ \
>> + return test_and_set_bit(EXT4_GROUP_INFO_##statename##_BIT, \
>> + &(grp->bb_state)); \
>> +}
>> +
>> +EXT4_MB_GROUP_STATE_FUNCS(need_init, NEED_INIT)
>> +EXT4_MB_GROUP_STATE_FUNCS(trimmed, WAS_TRIMMED)
>> +EXT4_MB_GROUP_STATE_FUNCS(bbitmap_corrupt, BBITMAP_CORRUPT)
>> +EXT4_MB_GROUP_STATE_FUNCS(ibitmap_corrupt, IBITMAP_CORRUPT)
>
> One problem with macros like this that internally expand to multiple
> functions is that there is now nowhere in this code where, for example,
> the declaration of ext4_mb_grp_test_and_set_bbitmap_corrupt() can be
> found. That makes it hard to understand the code, because tags for this
> function name will not work, and even a grep through the entire code for
> this string will not show the function implementation, only users. One
> would have to search for only the "ext4_mb_grp_test_and_set" part, or
> "ext4_mb_grp_clear" to find the above macros.
>
> If such macros-that-generate-functions are being used, my preference is
> that at least a comment block is added that spells out the full function
> names, so that at least a grep will find them, like:
>
> /*
> * These macros implement the following functions:
> * - ext4_mb_grp_need_init(), ext4_mb_grp_test_and_set_need_init(),
> * ext4_mb_grp_set_need_init(), ext4_mb_grp_clear_need_init()
> * - ...
> * - ...
> */
>
> Yes, this is a bit cumbersome the rare times a new function is added, but
> it really makes the code easier to understand in the future, without forcing
> a cut-and-paste of the body of each function. I don't know how many times
> I've had to search for commonly-used functions like buffer_uptodate() or
> buffer_dirty() in the code without being able to find them easily.
>
Thanks for your comments. Indeed, I also had the same hard time as you said.
I am not so sure why we have been using these maco functions for ext4 features
and ext4_inode_info bit flags. But I think it's still worth to unify them.
I will add the comment block as your suggested and post the second version,
BTW, I read the commit 3f61c0cc706 "ext4: add prototypes for macro-generated
functions" you posted, it's also a good choice.
Thanks,
Yi.
