Re: [PATCH 1/3] btrfs: introduce btrfs_find_inode

[Filipe Manana <fdmanana@kernel.org>]

On Thu, Aug 04, 2022 at 06:08:06PM +0200, David Sterba wrote:
> On Thu, Aug 04, 2022 at 04:52:21PM +0100, Filipe Manana wrote:
> > On Thu, Aug 04, 2022 at 05:28:24PM +0200, David Sterba wrote:
> > > On Thu, Jul 21, 2022 at 04:50:04PM +0300, Nikolay Borisov wrote:
> > > 
> > > Please use the simplified format that we have in btrfs.
> > > 
> > > > + *
> > > > + * @root:      root which is going to be searched for an inode
> > > > + * @objectid:  ino being searched for, if no exact match can be found the
> > > > + *             function returns the first largest inode
> > > > + *
> > > > + * Returns the rb_node pointing to the specified inode or returns NULL if no
> > > > + * match is found.
> > > > + *
> > > > + */
> > > > +struct rb_node *btrfs_find_inode(struct btrfs_root *root, const u64 objectid)
> > > 
> > > Const arguments for int types does not make sense.
> > 
> > It makes sense to me, as much as declaring local variables as const, and I don't
> > recall you ever complain about local const variables before (I do it often, and
> > I'm not the only one).
> 
> The function parameters are supposed to be set by callers and what's in
> the prototype is contract. A const pointer says "callee will not change
> this, promise", but for integer types it does not make sense because it
> does not establish any guarantees to caller.

Sure, but my point was not about giving guarantees to the caller.
It was all about readability for someone reading and changing code.

> 
> Local variables completely live inside a function and adding the const
> there can in some cases optimize the code so that compiler does not need
> to read the memory repeatedly. We've been adding it to the known
> constant values like sectorsize, or when there's a feature bit or other
> status information that's clearly unchanged during the function.
> 
> > Once I read the const part, I can tell for sure that nowhere in the function the
> > value of the argument is changed.
> 
> > It happens often that large functions use an int argument as if it was a local
> > variable and change its value later on, which makes reading the code often a bit
> > more time consuming and often leads to mistakest too.
> 
> I'd rather make this an exception than a rule, to avoid mistakes and for
> clarity that a long function takes certain input, but not for short
> functions.

Not sure if I understood that correctly.
You mean it's ok to add the const qualifier only if the function is long?