[PATCH] D107873: [clang-tidy] Add 'performance-const-parameter-value-or-ref' for checking const-qualified parameters

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.h:40
+  static constexpr int DefaultSmallMaxSize = 16;
+  int SmallMaxSize = Options.get("SmallMaxSize", DefaultSmallMaxSize);
+
----------------
jmarrec wrote:
> 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
Could you please elaborate on why you find this interesting? I don't see any benefit to ignoring `struct S { char c; }`.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst:44
+  // warning: 'const int' is a small, trivially copyable type: pass by value instead
+  // fix-it: void f6(const int i) {}
+
----------------
jmarrec wrote:
> 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...
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107873/new/

https://reviews.llvm.org/D107873