[PATCH] D88213: [llvm-objcopy][NFC] refactor error handling. part 2.

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added a subscriber: rsmith.
alexshap added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp:57
   support::endian::write32le(Data.data() + CRCPos, CRC32);
-  return Data;
+  return std::move(Data);
 }
----------------
grimar wrote:
> alexshap wrote:
> > avl wrote:
> > > grimar wrote:
> > > > avl wrote:
> > > > > grimar wrote:
> > > > > > You don't need this move I believe. Move constructor of `Expected<>` will be called anyways.
> > > > > It looks like in gcc case it does not work  - https://reviews.llvm.org/rG4da6927de47074f56531c2e7e2eecc4d6a1f09ec
> > > > Have you checked this place? The commit message does not say what was the BB error.
> > > > But in LLVM code we have many places that don't have `std::move` and feel fine.
> > > > 
> > > > https://github.com/llvm/llvm-project/blob/master/llvm/lib/Object/ArchiveWriter.cpp#L510
> > > > https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-exegesis/llvm-exegesis.cpp#L287
> > > > https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-readobj/ELFDumper.cpp#L582
> > > > 
> > > > Perhaps, the issue was related to `std::unique_ptr`.
> > > > 
> > > > 
> > > yes, the error was about unique_ptr:
> > > 
> > > MachOReader.cpp:337:10: error: could not convert ‘Obj’ from ‘std::unique_ptr<llvm::objcopy::macho::Object>’ to ‘llvm::Expected<std::unique_ptr<llvm::objcopy::macho::Object> >’
> > >    return Obj;
> > @grimar:
> > 
> > https://reviews.llvm.org/D43322
> > 
> > if there is a mismatch of the return type and the type of the local variable then std::move can be useful
> > 
> > if I am not mistaken, in fact, https://github.com/llvm/llvm-project/blob/master/llvm/lib/Object/ArchiveWriter.cpp#L510 is not a good example.
> > if there is a mismatch of the return type and the type of the local variable then std::move can be useful
> 
> `Expected<>` has a move constructor that is expected to handle the case here I believe (I've tested that MSVS 2017 calls it):
> 
> 
> ```
>   template <typename OtherT>
>   Expected(OtherT &&Val,
>            std::enable_if_t<std::is_convertible<OtherT, T>::value> * = nullptr)
>       : HasError(false)
> #if LLVM_ENABLE_ABI_BREAKING_CHECKS
>         // Expected is unchecked upon construction in Debug builds.
>         ,
>         Unchecked(true)
> #endif
>   {
>     new (getStorage()) storage_type(std::forward<OtherT>(Val));
>   }
> ```
> 
> My point mostly is based on the fact that using of `std::move` when returning a named local variable is generally should be avoided, because might affect compiler optimizations.
Apart from the diff which I mentioned above (D43322) there is also
D70963  which contains some extra context.

I'm not really sure what the current policy is / would be interesting to find out (perhaps, it depends on which compiler versions LLVM's build currently supports), but I'm not the best person to answer this question.
cc: @rsmith 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88213