[PATCH 16/21] erofs: kill magic underscores

[Gao Xiang]

Hi Christoph,

On Mon, Sep 02, 2019 at 05:26:27AM -0700, Christoph Hellwig wrote:
> >  
> > -	vi->datamode = __inode_data_mapping(advise);
> > +	vi->datamode = erofs_inode_data_mapping(advise);
> >  
> >  	if (vi->datamode >= EROFS_INODE_LAYOUT_MAX) {
> 
> While you are at it can we aim for some naming consistency here?  The
> inode member is called is called datamode, the helper is called
> inode_data_mapping, and the enum uses layout?  To me data_layout seems
> most descriptive, datamode is probably ok, but mapping seems very
> misleading given that we've already overloaded that terms for multiple
> other uses.

the naming changed for many times...
Initially, it was called "data_mapping_mode", and I think it was too long
(and as you said confusing, since confusing "mapping" meaning, sorry about
my awkward  English) so I simplified some into datamode....

In a word, I will change all data_mapping_mode into datalayout....

> 
> And while we are at naming choices - maybe i_format might be
> a better name for the i_advise field in the on-disk inode?

That is a good idea, I will resend v2 to address it...

> 
> > +	if (erofs_inode_version(advise) == EROFS_INODE_LAYOUT_V2) {
> 
> I still need to wade through the old thread - but didn't you say this
> wasn't really a new vs old version but a compat vs full inode?  Maybe
> the names aren't that suitable either?

Could you kindly give some suggestions for better naming about it?
there are all supported by EROFS... (Actually we only mainly use v1...)

> 
> >  		struct erofs_inode_v2 *v2 = data;
> >  
> >  		vi->inode_isize = sizeof(struct erofs_inode_v2);
> > @@ -58,7 +58,7 @@ static int read_inode(struct inode *inode, void *data)
> >  		/* total blocks for compressed files */
> >  		if (erofs_inode_is_data_compressed(vi->datamode))
> >  			nblks = le32_to_cpu(v2->i_u.compressed_blocks);
> > -	} else if (__inode_version(advise) == EROFS_INODE_LAYOUT_V1) {
> > +	} else if (erofs_inode_version(advise) == EROFS_INODE_LAYOUT_V1) {
> 
> Also a lot of code would use a switch statements to switch for different
> matches on the same value instead of chained if/else if/else if
> statements.

I will change it as well.

> 
> > +#define erofs_bitrange(x, bit, bits) (((x) >> (bit)) & ((1 << (bits)) - 1))
> 
> > +#define erofs_inode_version(advise)	\
> > +	erofs_bitrange(advise, EROFS_I_VERSION_BIT, EROFS_I_VERSION_BITS)
> >  
> > +#define erofs_inode_data_mapping(advise)	\
> > +	erofs_bitrange(advise, EROFS_I_DATA_MAPPING_BIT, \
> > +		       EROFS_I_DATA_MAPPING_BITS)
> 
> All these should probably be inline functions.

Will fix...

Thanks,
Gao Xiang