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.  | |
| 73 | QualType is small (AFAIK it's just 2 pointers), no need for a reference on this, it can live on the stack.  | |
| 92 | This reminds me eerily of performance-unnecessary-value-param!  | |
| clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.h | ||
| 19–22 | ||
| clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst | ||
| 59 | (Minor suggestion: maybe MaxSmallSize would be a better name here than SmallMaxSize.)  | |
| 102–103 | 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 | ||
|---|---|---|
| 40 | 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  | |
| 43 | 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 | ||
| 44 | 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 | ||
|---|---|---|
| 40 | 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 | ||
| 44 | 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.