[PATCH] D74692: [clang-tidy] Make bugprone-use-after-move ignore std::move for const values

[Arthur O'Dwyer via cfe-commits]

I see your point about how users who care should always be passing this
check alongside "performance-move-const-arg"; but IMVHO it still makes
sense for clang-tidy to warn unconditionally about code of the form

    x = std::move(y);
    use(y);

regardless of the incidental type of `y`. Sure, it's *technically* not
wrong if `y` is const... or if `y` is a primitive type such as `Widget*`...
or if `y` is a well-defined library type such as
`std::shared_ptr<Widget>`... or if `y` is a library type that works in
practice, such as `std::list<int>`
<https://stackoverflow.com/questions/60175782/is-stdlistint-listreturn-stdmovelistlist-guaranteed-to-leave/60186200#comment106478458_60186200>...
but regardless of its *technical* merits, the code is still *logically
semantically* wrong, and I think that's what the clang-tidy check should be
trying to diagnose.

my $.02,
Arthur

On Sun, Feb 16, 2020 at 10:44 AM Zinovy Nis via Phabricator via cfe-commits
<cfe-commits at lists.llvm.org> wrote:

> zinovy.nis updated this revision to Diff 244878.
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D74692/new/
>
> https://reviews.llvm.org/D74692
>
> Files:
>   clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
>   clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
>
>
> Index:
> clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
> ===================================================================
> --- clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
> +++ clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
> @@ -129,6 +129,14 @@
>    // CHECK-NOTES: [[@LINE-3]]:15: note: move occurred here
>  }
>
> +// Simple case for const value: no actual move and no warning.
> +void simpleConst(const A a) {
> +  A other_a = std::move(a);
> +  a.foo();
> +  // CHECK-NOTES-NOT: [[@LINE-1]]:3: warning: 'a' used after it was moved
> +  // CHECK-NOTES-NOT: [[@LINE-3]]:15: note: move occurred here
> +}
> +
>  // A warning should only be emitted for one use-after-move.
>  void onlyFlagOneUseAfterMove() {
>    A a;
> @@ -308,14 +316,15 @@
>  }
>
>  void lambdas() {
> -  // Use-after-moves inside a lambda should be detected.
> +  // Use-after-moves inside a lambda will not be detected as [a]
> +  // is passed as a const by default.
>    {
>      A a;
>      auto lambda = [a] {
>        std::move(a);
>        a.foo();
> -      // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved
> -      // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here
> +      // CHECK-NOTES-NOT: [[@LINE-1]]:7: warning: 'a' used after it was
> moved
> +      // CHECK-NOTES-NOT: [[@LINE-3]]:7: note: move occurred here
>      };
>    }
>    // This is just as true if the variable was declared inside the lambda.
> @@ -720,15 +729,15 @@
>      // const pointer argument.
>      const A a;
>      std::move(a);
> -    passByConstPointer(&a);
> -    // CHECK-NOTES: [[@LINE-1]]:25: warning: 'a' used after it was moved
> -    // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
> +    passByConstPointer(&a); // expect no warning: the const value was not
> moved.
> +    // CHECK-NOTES-NOT: [[@LINE-1]]:25: warning: 'a' used after it was
> moved
> +    // CHECK-NOTES-NOT: [[@LINE-3]]:5: note: move occurred here
>    }
>    const A a;
>    std::move(a);
> -  passByConstReference(a);
> -  // CHECK-NOTES: [[@LINE-1]]:24: warning: 'a' used after it was moved
> -  // CHECK-NOTES: [[@LINE-3]]:3: note: move occurred here
> +  passByConstReference(a); // expect no warning: the const value was not
> moved.
> +  // CHECK-NOTES-NOT: [[@LINE-1]]:24: warning: 'a' used after it was moved
> +  // CHECK-NOTES-NOT: [[@LINE-3]]:3: note: move occurred here
>  }
>
>  // Clearing a standard container using clear() is treated as a
> Index: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
> ===================================================================
> --- clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
> +++ clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
> @@ -404,7 +404,8 @@
>                 hasArgument(0, declRefExpr().bind("arg")),
>                 anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
>                       hasAncestor(functionDecl().bind("containing-func"))),
> -               unless(inDecltypeOrTemplateArg()))
> +               unless(anyOf(inDecltypeOrTemplateArg(),
> +                            hasType(qualType(isConstQualified())))))
>            .bind("call-move");
>
>    Finder->addMatcher(
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200216/e83578c0/attachment.html>