[PATCH] D28022: [clang-tidy] Handle constructors in performance-unnecessary-value-param

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.


================
Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:96
 
-  // Do not trigger on non-const value parameters when:
-  // 1. they are in a constructor definition since they can likely trigger
-  //    modernize-pass-by-value which will suggest to move the argument.
-  if (!IsConstQualified && (llvm::isa<CXXConstructorDecl>(Function) ||
-                            !Function->doesThisDeclarationHaveABody()))
-    return;
+  const FunctionDecl &Definition = *Function->getDefinition();
 
----------------
aaron.ballman wrote:
> malcolm.parsons wrote:
> > aaron.ballman wrote:
> > > malcolm.parsons wrote:
> > > > malcolm.parsons wrote:
> > > > > aaron.ballman wrote:
> > > > > > Instead of using `hasBody()` and `getDefinition()`, you should use `FunctionDecl::getBody()` and pass in an argument to receive the function's definition.
> > > > > I don't want the body - the `CXXCtorInitializer`s are not in it.
> > > > I should use `hasBody()` and pass in an argument.
> > > You are calling `Function-hasBody()` followed by `Function->getDefinition()`, which is what `FunctionDecl::getBody()` does. The difference is that `FunctionDecl::getBody()` will also load serialized bodies from the AST (if the body happens to be in a PCH, for instance).
> > Do I want to load serialized bodies?
> I would imagine you would want to deserialize, since you care about the contents of the body, not just the presence of one. This may or may not be important with modules (Richard Smith would be able to give a more definitive answer there).
I would suggest writing minimal sane code that works until we have good reasons to write more complex code to support module-enabled builds (and appropriate tests).


================
Comment at: clang-tidy/utils/DeclRefExprUtils.h:51
+
+/// Returns true if DeclRefExpr is the argument of a copy-constructor call
+/// expression within Decl.
----------------
Please enclose `true`, `DeclRefExpr` and other inline code snippets in double backquotes.


https://reviews.llvm.org/D28022