Checks `const`-qualified parameters to determine if they should be
passed by value or by reference-to-`const` for function definitions.
Ignores proper function declarations, parameters without `const` in
their type specifier, and dependent types.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Work left to do in this patch: add user options. Figured getting a non-config version up for review first would be a good idea.
Those CI failures are kinda embarrassing. Is there any way to leverage clang-format in a case like this?
clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.cpp | ||
---|---|---|
58 | Please use static, not anonymous namespace for functions. | |
59 | Please don't use auto unless type is spelled in same statement or iterator. | |
clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.h | ||
2 | See other headers as example for C++ definition for code editors. | |
19 | Please separate with newline. | |
44 | Seems to be unused. | |
47 | Please separate with newline. | |
clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst | ||
17 | Please remove newline. | |
68 | Please use single back-tick for option values. |
- adds user options
- removes use of auto
- fixes header comment
- removes unused type
- fixes documentation
clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.cpp | ||
---|---|---|
58 | Hmm... there are lots of instances of namespace { in clang-tidy. | |
clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst | ||
105 | I think this is something I put at the end first, and then moved to the start of the sentence, but forgot to delete. |
clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.cpp | ||
---|---|---|
58 | Yeah, because you can't do static class/struct X. | |
74 | QualType is small (AFAIK it's just 2 pointers), no need for a reference on this, it can live on the stack. | |
93 | This reminds me eerily of performance-unnecessary-value-param! | |
clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.h | ||
20–23 | ||
clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst | ||
60 | (Minor suggestion: maybe MaxSmallSize would be a better name here than SmallMaxSize.) | |
103–104 | Will GitHub and Godbolt outlive LLVM? I am not comfortable with depending on external sources (such as shortlinks like that git.io one that I was a bit anxious to click...) to explain decisions. I get it that the code is long to be put into the documentation, though... But perhaps the benchmark's tables could go in here, at the very bottom, as an appendix. |
It seems like there may be some considerable overlap between this check and the one proposed in https://reviews.llvm.org/D54943. I know the other check is more about C++ Core Guidelines so it's not perfect overlap. But I do wonder if it'd make sense to combine the efforts given that the goal of both checks is to find const correctness issues. Do you think there's a chance for sharing implementation efforts here?
Imported some discussion from https://reviews.llvm.org/D107900#inline-1029358. Sorry it took that long
clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.h | ||
---|---|---|
41 | https://reviews.llvm.org/D107900#inline-1029358 On mine, I had another option RestrictToBuiltInTypes which I find an interesting option. It has a builtinType() matcher in the registerMatchers function | |
44 | Perhaps two additional options to turn off either "side" would be helpful? eg CheckPassByValueIsAppropriate and CheckPassByRefIsAppropriate, both defaulted to true | |
clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst | ||
45 | https://reviews.llvm.org/D107900#inline-1029358 I am removing the const as well in addition to the &. I guess you may be just relying on using it in conjunction with readability-avoid-const-params-in-decls but I think that makes it harder to use (you can't just do -checks=-*,<yourcheck> you have to remember to add the readability-avoid-const-params-in-decls too...). Taht being said, perhaps that's how it's meant to be... |
clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.h | ||
---|---|---|
41 | Could you please elaborate on why you find this interesting? I don't see any benefit to ignoring struct S { char c; }. | |
clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst | ||
45 | I don't know if our goals are aligned here. This patch intends to preserve constness in all cases. If a user defines a function as void f(int const& x) { } Then there's a good chance that they want the constness. Removing that constness changes the meaning of the program since x would then be modifiable. What's deferred to readability-avoid-const-params-in-decls is const-qualified value parameters in forward declarations, which isn't really in scope here, since forward declarations are used to overrule this check's analysis. |
See other headers as example for C++ definition for code editors.