-
-
Notifications
You must be signed in to change notification settings - Fork 313
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lib: Reduce scope of 'len' variable in list_gp.c #3988
Conversation
@@ -36,6 +36,7 @@ int I_list_group(const char *group, const struct Ref *ref, FILE *fd) | |||
} | |||
max = 0; | |||
for (i = 0; i < ref->nfiles; i++) { | |||
int len; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is having an variable initialized inside a for loop a good practice in C? Usually I would see more efforts to take out the initialization and definition of what's inside the loop to put it outside the loop. The only other thing I know that would come close to what you did, is when using accelerator pragmas, like OpenACC, OpenMP, or MPI, where knowing that a variable is scoped to a loop iteration allows the loop iterations to be separated between devices. I don't think this is the case here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your feedback. I noticed the warning in cppcheck and aimed to silence it by limiting the scope of the len variable. However, I understand that declaring variables inside a loop may introduce memory overhead and affect performance, especially in performance-critical code or parallel computing contexts.
Given these considerations, I will close the pull request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how bad is having a scope bigger than needed, but are they other ways to limit the scope (are extra braces allowed in C?) On the other side, I remember being taught at school that having variables declared altogether at the top (whatever the language) was a good practice (at least for beginners).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe reducing the scope of variables can enhance code readability and maintainability by making it clear where each variable is used. Additionally, cppcheck warnings often highlight variables that are declared but not used, which can be addressed by limiting their scope. Doing this can contribute to more efficient and readable code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a similar but different line of thought, what's your opinion on clang-tidy? Does it find similar issues to cppcheck? What's his strengths, or when would you want to use that tool? Does it suggest fixes too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are correct that clang-tidy is also a static analysis tool and it finds similar issues to cppcheck. I use both tools because they complement each other. Using both of them gives more comprehensive coverage of issues and warnings that can be fixed.
Sometimes they identify similar issues, but often they catch different problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is having an variable initialized inside a for loop a good practice in C?
In modern C yes. It is a clear signal to the compiler that it can use any optimization tricks it knows. If you'll look into the produced code, quite often those inner variables do not exist at all as they have been optimized away.
But I am not a big fan of this PR for a different reason – we have a lot of places where this kind of micro-optimization can be done. Unless it is in a really tight loop, it does not make much sense to fix them alone without enhancing the rest of code.
Overview:
This pull request reduces the scope of the variable 'len' in the
list_gp.c
file. The variable 'len' is now declared inside the loops where it is used.Issue:
cppcheck identified the following issue:
list_gp.c:30:9: :The scope of the variable 'len' can be reduced. [variableScope]
int len, tot_len;
^
Solution:
The
len
variable has been moved inside the for loop where it is used. This change reduces its scope.