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
