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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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();
 
----------------
alexfh wrote:
> 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).
I think either approach is minimal, sane code and we should understand why we shouldn't use the interface provided by `FunctionDecl::getBody()` for getting the definition before deciding to not use it.

I *think* we're fine to not use `getBody()` because the matcher should match over all of the function declarations with a definition, and so eventually it will find the definition with the actual body. In the case that the function definition is serialized, I think this already does the right thing and deserializes it. (I think the only case where we need the deserialization may be when we have a function declaration and we need to traverse from that to the function body, which may be in a different `FunctionDecl`.) So the use of `hasBody()` in the matcher above seems like the correct solution to me.


https://reviews.llvm.org/D28022