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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
