[PATCH] D107873: [clang-tidy] adds a const-qualified parameter check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.cpp:73
+
+  const QualType &T = Param.getType();
+
----------------
QualType is small (AFAIK it's just 2 pointers), no need for a reference on this, it can live on the stack.


================
Comment at: clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.cpp:92
+    diag(Loc,
+         "%0 is not trivially copyable: pass by reference-to-const instead")
+        << T << Hint;
----------------
This reminds me eerily of [[ http://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-value-param.html | performance-unnecessary-value-param ]]!


================
Comment at: clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.cpp:57
+namespace {
+bool isSharedPtr(const QualType &T) {
+  if (auto R = T->getAsCXXRecordDecl())
----------------
cjdb wrote:
> Eugene.Zelenko wrote:
> > Please use `static`, not anonymous namespace for functions.
> Hmm... there are lots of instances of `namespace {` in clang-tidy.
Yeah, because you can't do `static class/struct X`.


================
Comment at: clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.h:19-22
+// 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.
----------------



================
Comment at: clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst:59
+
+.. option:: SmallMaxSize
+
----------------
(Minor suggestion: maybe MaxSmallSize would be a better name here than SmallMaxSize.)


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst:102-103
+determine when passing by value was no longer competetive with passing by
+reference for various `boards <https://git.io/JRKGv>`_. The benchmark source can
+be found on `Compiler Explorer <https://godbolt.org/z/rfW1Wqajh>`_.
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107873