Szelethus added a comment. In https://reviews.llvm.org/D54557#1300581, @NoQ wrote:
> In https://reviews.llvm.org/D54557#1299899, @Szelethus wrote: > > > Whenever I compile `Rtags`, I get a bunch of move related warnings, I'm > > curious how this patch behaves on that project. I'll take a look. > > > Whoops, sry, i accidentally had a look because i misread your message as if > you wanted me to take a look >.< > > (IMPORTANT) **Spoiler alert!** > > It finds one positive (and one duplicate of that one positive): > > F7553145: rtags-move-positive.html <https://reviews.llvm.org/F7553145> > > I believe this positive is a real bug, but we can still do better here. We > find it as a use-after-move of a local variable, which is not a bug on its > own, i.e. the user intended to empty and then re-use the storage and it's > fine, this is not a usual kind of typo where the user uses the pre-move > variable instead of the post-move variable. But the real bug here is that > this local variable uses an `std::string` as a field under the hood, which > is, well, not guaranteed to be empty after move, like all other STL objects: > https://stackoverflow.com/questions/27376623/can-you-reuse-a-moved-stdstring > > NOTE: As also mentioned in this stackoverflow thread, we also need to exclude > smart pointers from the STL check because they don't end up in unspecified > state, and see if there are other cornercases here. OMG That is cool. :D That project was fishy. Nice catch, would you like to make an issue on their project or shall I? > Unfortunately, we don't find this bug as an STL use-after-move bug because > inlining isn't happening. Why? I guess i'll leave it as an excercise ^.^ It's > a combination of running out of budget and a specific feature that we have. > By flipping a single `-analyzer-config` flag that represents that feature, we > are able to find such bugs as STL bugs (when local variable bugs are disabled > in the checker). We're still not able to find the original bug, most likely > due to running out of budget (i didn't debug this further), but we can find > it in a minimal example: > > #include "rct/Rct.h" > > void foo() { > String S1; > String S2 = std::move(S1); > S1 += "asdfg"; // use-after-move of a std::string > } > > > Here's the report that we are able to obtain for this trivial code snippet, > and you can look up the answer to the exercise in the collapsed run-line :) > > F7553290: rtags-move-positive-simplified.html > <https://reviews.llvm.org/F7553290> Thanks! :) *throws confetti* In https://reviews.llvm.org/D54557#1300653, @NoQ wrote: > In https://reviews.llvm.org/D54557#1299899, @Szelethus wrote: > > > I think we should either remove the non-default functionality (which > > wouldn't be ideal), or emphasise somewhere (open projects?) that there is > > still work to be done, but leaving it to be forgotten and essentially > > making it an extra maintenance work would be, in my optinion, the worst > > case scenario. `Aggressive` isn't `Pedantic` because it actually emits > > warnings on correct code, and it's not a simple matter of too many reports > > being emitted, let's also document that this is an experimental feature, > > not a power-user-only thing. > > > I only kept the option around because i was under an impression that i'm > intruding into a checker that already has some happy users, probably breaking > existing workflows. If this option is unnecessary, i'd be happy to remove it > :) Hmm, I'll ask around, but I'm not aware of any ongoing (or planned in the near future) work on this particular checker. Repository: rC Clang https://reviews.llvm.org/D54557 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
