RE: [PATCH] acl: fix build with GCC 15 on aarch64

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Konstantin Ananyev [mailto:konstantin.ananyev@huawei.com]
> Sent: Thursday, 27 March 2025 13.10
> 
> > > >
> > > > > From: David Marchand <david.marchand@redhat.com>
> > > > > Sent: Thursday, March 27, 2025 10:37 AM
> > > > >
> > > > > On Thu, Mar 27, 2025 at 9:55 AM Bruce Richardson
> > > > > <bruce.richardson@intel.com> wrote:
> > > > > >
> > > > > > On Wed, Mar 26, 2025 at 11:39:28AM +0100, David Marchand
> wrote:
> > > > > > > @@ -176,6 +176,8 @@ acl_set_flow(struct acl_flow_data
> *flows,
> > > struct completion *cmplt,
> > > > > > >       uint32_t cmplt_size, const uint8_t **data, uint32_t
> > > *results,
> > > > > > >       uint32_t data_num, uint32_t categories, const
> uint64_t
> > > *trans)
> > > > > > >  {
> > > > > > > +     unsigned int i;
> > > > > > > +
> > > > > > >       flows->num_packets = 0;
> > > > > > >       flows->started = 0;
> > > > > > >       flows->trie = 0;
> > > > > > > @@ -187,6 +189,9 @@ acl_set_flow(struct acl_flow_data
> *flows,
> > > struct completion *cmplt,
> > > > > > >       flows->data = data;
> > > > > > >       flows->results = results;
> > > > > > >       flows->trans = trans;
> > > > > > > +
> > > > > > > +     for (i = 0; i < cmplt_size; i++)
> > > > > > > +             cmplt[i].count = 0;
> > > > > > >  }
> > > > > >
> > > > > > Minor nit, but since we are using c11 standard, is it not
> better
> > > to declare
> > > > > > "i" inside the "for" statement. Keeps diffs simpler for
> > > adding/removing
> > > > > > code, I think.
> > > > >
> > > > > I still have this (bad) habit but yes, it looks nicer with
> > > declaring
> > > > > in for() itself.
> > > >
> > > > My vote would be to keep it in an old fashioned way.
> > > > Nothing is wrong in defining variable to use at the start of the
> > > function :)
> > > >
> > >
> > > No, there isn't. However, there is also a reason why later GCC
> > > revisions
> > > and modern languages allow use of a temporary variable defined
> within
> > > the
> > > loop itself. Cognitively, it's easier to have variables defined at
> > > point of
> > > use, as it saves the user having to mentally track them or move up
> and
> > > down the
> > > code. Furthermore, when debugging or reworking the code, it's far
> > > easier to
> > > have the variable inside the "for" statement as it means that as we
> > > comment/uncomment, or remove/re-add, the code block, the variable
> > > definition
> > > also gets commented/uncommented too, without having to constantly
> > > scroll up
> > > to make changes in two places. Lastly, it makes for smaller git
> diffs
> > > too.
> 
> I understand that it is probably more convenient, though from my
> perspective it is also more error prone.
> I saw several times people unintentionally defined new variable (in a
> local scope) with the same name
> that was already used in an outer scope, especially when function
> becomes large and clunky.

Yes, it used to be a problem, causing bugs that were difficult to find by the developer who wrote the code.
But modern compilers warn about that, so not a problem anymore.

> Personally, I think it is a good practice to do a 'mental track' of
> your variables when writing the code,
> and having all of them defined in one place definitely helps with that.

I partially agree with this. Variables declared in the middle of a code block are difficult to keep mental track of.

However, minimizing the scope of variables reduces the mental load when reviewing code.
If some variable is only used within a block of code, it should not be declared outside that block.

As always, there are exceptions to all rules. E.g. a local variable used at the end of a code block, e.g. as a helper for temporarily cleaning up, is OK to declare there.

> Anyway, I am not about to stop people to define variables inside the
> for() if it is more convenient for them,
> but I am against to force people to write code that way.

Coding style is a balance between readability/reviewability, bug prevention and preferences.

And with our current coding style, either way of declaring "i" in this case should be acceptable.

> 
> > >
> > > /Bruce
> >
> > I agree with Bruce.
> > Also, minimizing the scope of local variables reduces the risk of
> bugs caused by unintended reuse without re-initialization. Reducing
> > the risk of bugs is important.
> >
> > BTW: The Coding Style documentation [CODINGSTYLE] is still based on
> an ancient C version, and needs to be updated.
> >
> > [CODINGSTYLE]:
> https://doc.dpdk.org/guides/contributing/coding_style.html#local-
> variables