cjdb added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp:177-178
+ diag(NonMemberLoc, "ignoring the result of '%0', did you mean
'clear()'?")
+ << llvm::dyn_cast<NamedDecl>(NonMemberCall->getCalleeDecl())
+ ->getQualifiedNameAsString()
+ << FixItHint::CreateReplacement(ReplacementRange, ReplacementText);
----------------
abrahamcd wrote:
> njames93 wrote:
> > Diagnostics can accept args as `const NamedDecl *`, so you can omit the
> > call to `getQualifiedNameAsString()`.
> > The use of `dyn_cast` here is suspicious. If the CalleeDecl isn't a
> > `NamedDecl`, this would assert when you try and call
> > `getQualifiedNameAsString`, but equally I can't see any case why the
> > CalleeDecl wouldn't be a `NamedDecl`. @rsmith Can you think of any
> > situation where this could happen?
> Seems that without `getQualifiedNameAsString()` I get longer less-readable
> versions of the name, e.g. `'empty<std::vector<int> &>'` instead of just
> `'std::empty'`. Do you think the extra information is helpful here?
I'm partial to `'std::empty'` because that's presumably what the user will
type, and so it'll be easier for them to understand. `'empty<std::vector<int>
&>'` isn't useful because we almost always defer to type deduction.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp:57-72
+ using ast_matchers::callee;
+ using ast_matchers::callExpr;
+ using ast_matchers::cxxMemberCallExpr;
+ using ast_matchers::cxxMethodDecl;
+ using ast_matchers::expr;
+ using ast_matchers::functionDecl;
+ using ast_matchers::hasName;
----------------
njames93 wrote:
> These using declarations are annoying, the common convention in tidy checks
> is to just use a global `using namespace ast_matchers` at the top of the file.
Adding them just after the namespaces are opened would be good.
I've had issues with refactoring llvm-project files in the past that have
global //using-directives//, so I'd prefer it if we stuck with the
//using-declarations// please. If they're a part of the preamble, they become
additional lines that will be scrolled over.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp:120-121
+ llvm::find_if(Methods, [](const CXXMethodDecl *F) {
+ return F->getDeclName().getAsString() == "clear" &&
+ F->getMinRequiredArguments() == 0;
+ });
----------------
njames93 wrote:
> We also need to check qualifiers. If there exists a clear method but its
> unavailable due to it not being const when our member is const. The fix would
> result in a compile error.
Good catch. We probably shouldn't be suggesting `clear()` on a const object
anyway.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128372/new/
https://reviews.llvm.org/D128372
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
