> From: Konstantin Ananyev [mailto:konstantin.anan...@huawei.com] > Sent: Thursday, 27 March 2025 13.10 > > > > > > > > > > From: David Marchand <david.march...@redhat.com> > > > > > Sent: Thursday, March 27, 2025 10:37 AM > > > > > > > > > > On Thu, Mar 27, 2025 at 9:55 AM Bruce Richardson > > > > > <bruce.richard...@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
