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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to