mboehme marked 5 inline comments as done.
mboehme added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:396
void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
+ auto StandardMapMatcher =
+ hasType(hasUnqualifiedDesugaredType(recordType(hasDeclaration(
----------------
sammccall wrote:
> 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).
Thanks -- I wasn't sure myself whether I should be so restrictive, and you make
some very convincing points. Done!
================
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
----------------
sammccall wrote:
> 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.
I've stolen your wording -- thanks!
================
Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:409
+ // try_emplace.
+ unless(hasParent(cxxMemberCallExpr(
+ on(expr(declRefExpr(), StandardMapMatcher)),
----------------
sammccall wrote:
> 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)
Yeah, I thought about that myself but couldn't come up with anything.
================
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);
----------------
sammccall wrote:
> hahaha :-)
Yeah, I was being a bit sloppy with my definition of what a map is...
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
