sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Nice! Only substantive suggestion: I think requiring the base type to be
exactly a standard map type is too conservative.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:396
void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
+ auto StandardMapMatcher =
+ hasType(hasUnqualifiedDesugaredType(recordType(hasDeclaration(
----------------
I'd suggest dropping this condition.
Custom containers often emulate the standard library (e.g. llvm::DenseMap
implements try_emplace, as do the absl maps and I found several other examples).
It seems very unlikely to encounter a try_emplace function without these
semantics.
(The only case I can imagine is if for some reason they didn't bother to
implement the bool return value, so there was no way to know whether
emplacement happened. False negative there doesn't seem bad).
================
Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:406
+ unless(inDecltypeOrTemplateArg()),
+ // Ignore std::move()s inside a try_emplace() call to avoid
false
+ // positives as we can't reason about the bool returned by
----------------
Maybe explain why try_emplace is special?
try_emplace is a common maybe-moving function that returns a bool that tells
callers whether it moved.
Ignore std::move inside try_emplace to avoid false positives, as we don't track
uses of the bool.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:409
+ // try_emplace.
+ unless(hasParent(cxxMemberCallExpr(
+ on(expr(declRefExpr(), StandardMapMatcher)),
----------------
i'm trying to think if there's an important case where the call isn't the
immediate parent of the move and yet we're still maybe-moving... but I can't
think of one. (Just mentioning in case one occurs to you)
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/bugprone-use-after-move.rst:173
+There is one special case: A call to ``std::move`` inside a ``try_emplace``
call
+on a standard map type is ignored. This is to avoid spurious warnings, as the
+check has no way to reason about the ``bool`` returned by ``try_emplace``.
----------------
s/ignored/conservatively assumed *not* to move/?
(It wasn't obvious to me when reading how "ignored" in the implementation
translates to the domain)
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:510
+// Ignore moves that happen in a try_emplace.
+void try_emplace() {
+ {
----------------
calling this function exactly try_emplace seems gratuitiously confusing - since
the condition we're changing here is "calls inside a function named
try_emplace", just with a different definition of "inside"...
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:831
{
- std::map<int> container;
+ std::map<int, int> container;
std::move(container);
----------------
hahaha :-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98034/new/
https://reviews.llvm.org/D98034
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
