Skip to content
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

Codecheck: 'const' for a literal parameter ... does not catch all instances #1622

Closed
bunnybot opened this issue Sep 9, 2019 · 5 comments · Fixed by #5709
Closed

Codecheck: 'const' for a literal parameter ... does not catch all instances #1622

bunnybot opened this issue Sep 9, 2019 · 5 comments · Fixed by #5709
Assignees
Labels
bug Something isn't working codecheck Compiler warnings, clang-tidy, code style checks, linters, …
Milestone

Comments

@bunnybot
Copy link

bunnybot commented Sep 9, 2019

In this function:

void foo(const int bar,
                const int baz,
                const RGBColor& blah)

I will get the following codecheck error:

'const' for a literal parameter is an implementation detail. Remove the const from the interface definition.

However, with this function:

void foo(const int bar, const int baz, const RGBColor& blah)

no error is triggered.


Imported from Launchpad using lp2gh.

@bunnybot bunnybot added codecheck Compiler warnings, clang-tidy, code style checks, linters, … Low labels Sep 9, 2019
@gunchleoc gunchleoc added this to Needs triage in Issue triage Sep 13, 2019
@gunchleoc gunchleoc added the enhancement New feature or request label Sep 28, 2019
@gunchleoc gunchleoc removed this from To Do in Issue triage Sep 28, 2019
@Noordfrees
Copy link
Member

Noordfrees commented Oct 11, 2020

I get this one all the time. This false positive is sooo annoying…

E.g.: 8c502ef @ https://travis-ci.org/github/widelands/widelands/jobs/734548358#L3627

@Noordfrees Noordfrees added this to the v1.0 milestone Oct 11, 2020
@Noordfrees Noordfrees added bug Something isn't working and removed enhancement New feature or request labels Oct 11, 2020
@gunchleoc
Copy link
Contributor

It's not a false positive - the rule is performing there as it's intended to do.

I don't know what motivated this rule though. @SirVer do you remember?

@Noordfrees
Copy link
Member

It is a false positive when it catches the definition as well as the declaration. There is no reason why I shouldn't be allowed to make a literal parameter const in a .cc file, the rule should apply only to headers. Perhaps it should scan only .h files?

@Niektory
Copy link
Member

It seems the main problem is that it checks line by line, so it cannot distinguish declarations from definitions when they span multiple lines. Perhaps it shouldn't match any lines that don't end with a ;.

@gunchleoc
Copy link
Contributor

+1 for restricting it to .h files. Sorry, I overlooked that it was in a .cc file.

Perhaps it shouldn't match any lines that don't end with a ;.

Difficult for long function signatures that span multiple lines. We'd need to rewrite the rule to support multiline.

@Noordfrees Noordfrees removed this from the v1.0 milestone Jan 4, 2021
@Noordfrees Noordfrees self-assigned this Dec 12, 2022
@Noordfrees Noordfrees added this to the v1.2 milestone Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working codecheck Compiler warnings, clang-tidy, code style checks, linters, …
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants