fixed and enabled readability-avoid-const-params-in-decls clang-tidy warnings
#4861
Conversation
|
I filed https://trac.cppcheck.net/ticket/11602 for the These changes also triggered the already fixed https://trac.cppcheck.net/ticket/11535. |
cli/cppcheckexecutor.h
Outdated
| @@ -79,7 +79,7 @@ class CppCheckExecutor : public ErrorLogger { | |||
| /** xml output of errors */ | |||
| void reportErr(const ErrorMessage &msg) override; | |||
|
|
|||
| void reportProgress(const std::string &filename, const char stage[], const std::size_t value) override; | |||
| void reportProgress(const std::string &filename, const char stage[], std::size_t value) override; | |||
There was a problem hiding this comment.
I don't have a very strong opinion. But I don't see how it would hurt to keep these const.
There was a problem hiding this comment.
Well they have no effect and we also don't have the definition and declaration in sync. In some cases it might even be misleading. It also exposes bugs in our parsing as we are not properly handling this code.
There was a problem hiding this comment.
The point is to have const only in the implementation (where it matters), not in the declaration, right? So we don't actually lose anything here, since top-level const has no effect in a function declaration.
There was a problem hiding this comment.
it has no effect on the function interface. but it does have an effect in the function body.
The point is to have const only in the implementation (where it matters), not in the declaration, right?
In my opinion it would be a bad idea to have different const-qualifiers in the declaration and definition/implementation. Right?
There was a problem hiding this comment.
It has no effect. there's also cases where the header and source were not in sync and nothing will tell you. It actually doesn't matter at all. It just makes things "cleaner" - and exposes Cppcheck bugs :)
There was a problem hiding this comment.
It has no effect.
Well.. it does have an effect in the function body.
void foo(const int x) {
x=3; // <- not allowed because of const
}
I do not have a strong opinion if we should use const for parameters passed by value. But in my opinion we should not have different constness in the declaration and implementation. If the implementation uses const then I feel the declaration should too.
And as I said.. how does it hurt with const?
there's also cases where the header and source were not in sync and nothing will tell you.
Are you sure that this is explicitly permitted by the language standard? Can it be a compiler bug?
There was a problem hiding this comment.
Well.. it does have an effect in the function body.
That wasn't changed.
Are you sure that this is explicitly permitted by the language standard? Can it be a compiler bug?
Yes, it is allowed. No, it is not a bug. That's what this check is about: https://clang.llvm.org/extra/clang-tidy/checks/readability/avoid-const-params-in-decls.html
The following are the same as the top-level const has no effect:
void f(const int);
void f(int);There was a problem hiding this comment.
well.. I guess the language standard permits it then but I see no reference..
imho.. having different const in the declaration and implementation seems weird to me. And nobody seems to disagree.
There was a problem hiding this comment.
I don't see how it hurts to have the const. I do find it strange that people intentionally WANT to have different constness in the declaration and implementation.
There was a problem hiding this comment.
Long time lurker.
Interesting discussion in this stackoverflow question.
"Dropping const from function prototypes has the advantage that you don't need to alter the header file if you decide to drop const from implementation part later."
and
"It is all a matter of who needs which information. You provide the parameter by value so the caller does not need to know anything on what you do (internally) with it. ... In your implementation you do care that you can promise not to alter ..."
firewave
force-pushed
the
tidy-const
branch
2 times, most recently
from
75a253e
to
6f0b83e
Compare
firewave
force-pushed
the
tidy-const
branch
2 times, most recently
from
c7c8a9d
to
1d2d36c
Compare
|
conflict.. and I still feel a bit skeptic to have different constness in declaration/implementation. I don't see matching constness as bad style. |
firewave
force-pushed
the
tidy-const
branch
2 times, most recently
from
d0d7443
to
0bbf502
Compare
firewave
force-pushed
the
tidy-const
branch
2 times, most recently
from
c891783
to
dbc885e
Compare
No description provided.