[PATCH] D98034: [clang-tidy] Use-after-move: Ignore moves inside a try_emplace.

[Martin Böhme via Phabricator via cfe-commits]

mboehme added a comment.

First of all, sorry for submitting this change prematurely. I don't contribute to LLVM/Clang regularly and forgot that reviews should be kept open for a while after a reviewer has approved them to give others a chance to chime in. I jumped the gun here -- apologies.

In D98034#2606642 <https://reviews.llvm.org/D98034#2606642>, @njames93 wrote:

> In D98034#2606535 <https://reviews.llvm.org/D98034#2606535>, @mboehme wrote:
>
>> In D98034#2606488 <https://reviews.llvm.org/D98034#2606488>, @njames93 wrote:
>>
>>> While `try_emplace` is a special case, it's may not be the only special case in someone's codebase, this should be extended with options to let users handle their special containers.
>>
>> Sorry, I didn't see your comment before submitting.
>>
>> I agree that there may be other cases of "maybe move" functions, but try_emplace seems to be the most common case. Handling "maybe move" functions more general would require some way of annotating them; I felt it was useful to get the try_emplace case handled first.
>
> The canonical way of doing this in clang-tidy is to add an option to the check which takes a semi-colon delimited list.
> `clang::tidy::utils::options::parseStringList` can then turn that into a vector of string.
> Annoyingly the hasAnyName matcher needs a vector of StringRef's so most checks do something like this
>
>   hasAnyName(SmallVector<StringRef, 4>(
>         StringLikeClasses.begin(), StringLikeClasses.end())
>
> As you are reading options, you will need to override the storeOptions method in the check to then store the value read from config, `serializeStringList` is your friend here.
> The documentation will also need updating to describe the option and what it does.

I think I shouldn't just use a `functionDecl(hasAnyName())` matcher with a string list; here's why.

In the case of `try_emplace`, it makes sense to match on any method (or even a free function) with that name. I was originally restricting myself to only the standard map types, but as @sammccall pointed out to me, someone using this pretty unique name in their own code is very likely to be using the same semantics as the standard map types.

This may not be true for all methods that a user might want to specify though. Say a user has

  class MyClass {
  public:
    bool Insert(OtherClass &&);
  };

where, as for `try_emplace`, the boolean return value says whether the insertion actually happened.

We wouldn't simply want to ignore `std::move` arguments on any method called `Insert`. It's likely that there are other methods called `Insert` in the codebase that take an rvalue reference and move from it unconditionally, and we want the check to look at those.

So we'd want the user to be able to specify the qualified name `MyClass::Insert` as the function to ignore. This makes the matcher a bit more interesting as we'd need to work out when seeing a string like this whether `MyClass` is a class name or a namespace name. Or we might simply do something like

  anyOf(
      // maybe `MyClass` is a namespace...
      functionDecl(hasName("MyClass::Insert")),
      // ...or maybe it's a class
      cxxMethodDecl(ofClass(hasName("MyClass")), hasName("Insert"))
  )

I assume there isn't an existing matcher that does this -- or are you aware of one?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98034/new/

https://reviews.llvm.org/D98034