[nfc] Allow forwarding Error returns from Expected callers

[mtrofin]

On a few compilers (clang 11/12 for example [1]), the following does not result in a copy elision, and since Error's copy dtor is elided, results in a compile error:

  Expect<Something> foobar() {
    ...
    if (Error E = aCallReturningError())
      return E;
    ...
  }

Moving E would, conversely, result in the pessimizing-move warning on more recent clangs ("moving a local object in a return statement prevents copy elision")

We just need to make the Expected ctor taking an Error take it as a r-value reference.

[1] https://lab.llvm.org/buildbot/#/builders/54/builds/10505

[llvmbot]

@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-llvm-binary-utilities

Author: Mircea Trofin (mtrofin)

Changes

On a few compilers (clang 11/12 for example [1]), the following does not result in a copy elision, and since Error's copy dtor is elided, results in a compile error:

  Expect<Something> foobar() {
    ...
    if (Error E = aCallReturningError())
      return E;
    ...
  }

Moving E would, conversely, result in the pessimizing-move warning on more recent clangs warning ("moving a local object in a return statement prevents copy elision")

We just need to make the Expected ctor taking an Error take it as a r-value reference.

[1] https://lab.llvm.org/buildbot/#/builders/54/builds/10505

---

Full diff: https://github.com/llvm/llvm-project/pull/92208.diff

4 Files Affected:

• (modified) llvm/include/llvm/Support/Error.h (+1-1)
• (modified) llvm/lib/Bitstream/Reader/BitstreamReader.cpp (+6-6)
• (modified) llvm/lib/Object/COFFObjectFile.cpp (+3-3)
• (modified) llvm/lib/Object/WindowsResource.cpp (+2-2)

diff --git a/llvm/include/llvm/Support/Error.h b/llvm/include/llvm/Support/Error.h
index 894b6484336ae..217130ce293af 100644
--- a/llvm/include/llvm/Support/Error.h
+++ b/llvm/include/llvm/Support/Error.h
@@ -493,7 +493,7 @@ template <class T> class [[nodiscard]] Expected {
 
 public:
   /// Create an Expected<T> error value from the given Error.
-  Expected(Error Err)
+  Expected(Error &&Err)
       : HasError(true)
 #if LLVM_ENABLE_ABI_BREAKING_CHECKS
         // Expected is unchecked upon construction in Debug builds.
diff --git a/llvm/lib/Bitstream/Reader/BitstreamReader.cpp b/llvm/lib/Bitstream/Reader/BitstreamReader.cpp
index 3cc9dfdf7b858..5b2c76350029b 100644
--- a/llvm/lib/Bitstream/Reader/BitstreamReader.cpp
+++ b/llvm/lib/Bitstream/Reader/BitstreamReader.cpp
@@ -167,7 +167,7 @@ Expected<unsigned> BitstreamCursor::skipRecord(unsigned AbbrevID) {
         if (Error Err =
                 JumpToBit(GetCurrentBitNo() + static_cast<uint64_t>(NumElts) *
                                                   EltEnc.getEncodingData()))
-          return std::move(Err);
+          return Err;
         break;
       case BitCodeAbbrevOp::VBR:
         assert((unsigned)EltEnc.getEncodingData() <= MaxChunkSize);
@@ -180,7 +180,7 @@ Expected<unsigned> BitstreamCursor::skipRecord(unsigned AbbrevID) {
         break;
       case BitCodeAbbrevOp::Char6:
         if (Error Err = JumpToBit(GetCurrentBitNo() + NumElts * 6))
-          return std::move(Err);
+          return Err;
         break;
       }
       continue;
@@ -206,7 +206,7 @@ Expected<unsigned> BitstreamCursor::skipRecord(unsigned AbbrevID) {
 
     // Skip over the blob.
     if (Error Err = JumpToBit(NewEnd))
-      return std::move(Err);
+      return Err;
   }
   return Code;
 }
@@ -344,7 +344,7 @@ Expected<unsigned> BitstreamCursor::readRecord(unsigned AbbrevID,
     // over tail padding first, in case jumping to NewEnd invalidates the Blob
     // pointer.
     if (Error Err = JumpToBit(NewEnd))
-      return std::move(Err);
+      return Err;
     const char *Ptr = (const char *)getPointerToBit(CurBitPos, NumElts);
 
     // If we can return a reference to the data, do so to avoid copying it.
@@ -421,7 +421,7 @@ Error BitstreamCursor::ReadAbbrevRecord() {
 Expected<std::optional<BitstreamBlockInfo>>
 BitstreamCursor::ReadBlockInfoBlock(bool ReadBlockInfoNames) {
   if (llvm::Error Err = EnterSubBlock(bitc::BLOCKINFO_BLOCK_ID))
-    return std::move(Err);
+    return Err;
 
   BitstreamBlockInfo NewBlockInfo;
 
@@ -452,7 +452,7 @@ BitstreamCursor::ReadBlockInfoBlock(bool ReadBlockInfoNames) {
       if (!CurBlockInfo)
         return std::nullopt;
       if (Error Err = ReadAbbrevRecord())
-        return std::move(Err);
+        return Err;
 
       // ReadAbbrevRecord installs the abbrev in CurAbbrevs.  Move it to the
       // appropriate BlockInfo.
diff --git a/llvm/lib/Object/COFFObjectFile.cpp b/llvm/lib/Object/COFFObjectFile.cpp
index 18506f39f6b57..5a85b8e00c633 100644
--- a/llvm/lib/Object/COFFObjectFile.cpp
+++ b/llvm/lib/Object/COFFObjectFile.cpp
@@ -294,7 +294,7 @@ COFFObjectFile::getSectionContents(DataRefImpl Ref) const {
   const coff_section *Sec = toSec(Ref);
   ArrayRef<uint8_t> Res;
   if (Error E = getSectionContents(Sec, Res))
-    return std::move(E);
+    return E;
   return Res;
 }
 
@@ -807,7 +807,7 @@ Expected<std::unique_ptr<COFFObjectFile>>
 COFFObjectFile::create(MemoryBufferRef Object) {
   std::unique_ptr<COFFObjectFile> Obj(new COFFObjectFile(std::move(Object)));
   if (Error E = Obj->initialize())
-    return std::move(E);
+    return E;
   return std::move(Obj);
 }
 
@@ -1959,7 +1959,7 @@ ResourceSectionRef::getContents(const coff_resource_data_entry &Entry) {
     uint64_t Offset = Entry.DataRVA + Sym->getValue();
     ArrayRef<uint8_t> Contents;
     if (Error E = Obj->getSectionContents(*Section, Contents))
-      return std::move(E);
+      return E;
     if (Offset + Entry.DataSize > Contents.size())
       return createStringError(object_error::parse_failed,
                                "data outside of section");
diff --git a/llvm/lib/Object/WindowsResource.cpp b/llvm/lib/Object/WindowsResource.cpp
index 983c8e30a9420..306e8ec542068 100644
--- a/llvm/lib/Object/WindowsResource.cpp
+++ b/llvm/lib/Object/WindowsResource.cpp
@@ -80,7 +80,7 @@ Expected<ResourceEntryRef>
 ResourceEntryRef::create(BinaryStreamRef BSR, const WindowsResource *Owner) {
   auto Ref = ResourceEntryRef(BSR, Owner);
   if (auto E = Ref.loadNext())
-    return std::move(E);
+    return E;
   return Ref;
 }
 
@@ -1006,7 +1006,7 @@ writeWindowsResourceCOFF(COFF::MachineTypes MachineType,
   Error E = Error::success();
   WindowsResourceCOFFWriter Writer(MachineType, Parser, E);
   if (E)
-    return std::move(E);
+    return E;
   return Writer.write(TimeDateStamp);
 }
 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[snehasish]

lgtm

[snehasish]

nit: revert this formatting change?

[mtrofin]

I thought I did - in any case, it may upset the clang-format CI, but I agree formatting should happen in a separate patch.

[dwblaikie]

Perhaps worth some unit test coverage demonstrating this.

[mtrofin]

Perhaps worth some unit test coverage demonstrating this.

Followed up in #92314