[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Aaron Gokaslan via Phabricator via cfe-commits]

Skylion007 added a comment.

In D137205#3913192 <https://reviews.llvm.org/D137205#3913192>, @Febbe wrote:

> In D137205#3912243 <https://reviews.llvm.org/D137205#3912243>, @Skylion007 wrote:
>
>> One other bug I found with this diff, is it seems to suggest calling std::move() on function args that are references, despite the fact that invalidating the reference to the input arg could be undesirable. For instance take a function that takes in a reference to a String, assigns a new value to the arg reference, and then returns the value of the reference. It suggests calling std::move() on the arg ref when it is returned, which invalidate the reference to the argument.
>
> Oh, that's not good. Actually, it should not run on references, only  `declRefExpr(hasType(qualType(unless(anyOf(isConstQualified(), referenceType(), pointerType() ,..)` are allowed to be parsed.
>
> Did you run this check alone, without other checks?
>
> Regarding the appearance of `std::move(std::move( var );`, was there a move already? Does it occur in a header file? In case of running clang-tidy with fixups in parallel over multiple sources, it can happen, that a header file is parsed twice at the same time, resulting in duplicate insertions.
>
> And last but not least, can you provide a source snipped or AST for both appearances?

The problematic code snippit can be found here: https://github.com/facebookresearch/habitat-sim/blob/c88851802f1905ab483e380367753dc6a12c6d21/src/esp/metadata/managers/AbstractObjectAttributesManagerBase.h#L323

And results in this diff:

  diff --git a/src/esp/metadata/managers/AbstractObjectAttributesManagerBase.h b/src/esp/metadata/managers/AbstractObjectAttributesManagerBase.h
  index d5564c8f..37a38b00 100644
  --- a/src/esp/metadata/managers/AbstractObjectAttributesManagerBase.h
  +++ b/src/esp/metadata/managers/AbstractObjectAttributesManagerBase.h
  @@ -10,6 +10,8 @@
    * esp::metadata::managers::AbstractObjectAttributesManager
    */
  
  +#include <utility>
  +
   #include "esp/metadata/attributes/ObjectAttributes.h"
  
   #include "esp/metadata/MetadataUtils.h"
  @@ -312,7 +314,7 @@ AbstractObjectAttributesManager<T, Access>::setJSONAssetHandleAndType(
       const std::function<void(int)>& meshTypeSetter) {
     std::string propertiesFileDirectory = attributes->getFileDirectory();
     // save current file name
  -  std::string oldFName(assetName);
  +  std::string oldFName(std::move(assetName));
     // clear var to get new value - if returns true use this as new value
     assetName = "";
     // Map a json string value to its corresponding AssetType if found and cast to
  @@ -362,7 +364,7 @@ AbstractObjectAttributesManager<T, Access>::setJSONAssetHandleAndType(
                                              meshTypeSetter);
         }
       }  // value is valid prim and exists, else value is valid file and exists
  -    return assetName;
  +    return std::move(assetName);
     }
     // handle value is not present in JSON or is specified but does not change
     return oldFName;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205