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

[George Rimar via Phabricator via llvm-commits]

grimar 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);
 }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88213