-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
fix: syntax on display_icon #6935
Conversation
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.
AFAIK, sub prototypes are discouraged, and @_
is faster than shift
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.
Looks good, but could you also add a documentation header for the function, to explain what it does, what it returns etc. ?
bc7a016
to
2088cdb
Compare
Kudos, SonarCloud Quality Gate passed! |
How does this look? |
I've read the discussions like https://stackoverflow.com/questions/297034/why-are-perl-5s-function-prototypes-bad but the prototypes we currently have to specify the number of arguments have been useful (at least to me) in the past. e.g. when I added a parameter to a function, but forgot to change all the calls to add the parameter. It's clearly not perfect, but making sure we call functions with the right number of parameters is better than nothing I think. Well maybe it's time to start to use Perl's function signatures. :) |
I've tried reading about prototypes and they seem to cause specific argument setups to be passed to another function, but I ultimately don't understand it. It seems like I should revert the syntax changes in this PR and leave the comments. Does that sound right? |
@dipietroR It's a complex topic indeed. @hangy I'm going to merge this PR as-is, as it makes the code more consistent. But I opened a new bug to discuss prototypes and signatures: #6956 |
This function was using a different syntax than what appears to be used in most other functions. This change updates to both require one argument (this seems to be a requirement). And was using @_ rather than shift.