[Clang] Use std::move in GlobalModuleIndex::readIndex. NFC

[junaire]

Summary

BitstreamCursors are heavy-weight objects that should not be passed by value.

[RKSimon]

clang/lib/Serialization/GlobalModuleIndex.cpp

130

You're not passing this as a const, so the calling function's Cursor will be updated - this doesn't sound generally safe to me - what about a move?

[junaire]

clang/lib/Serialization/GlobalModuleIndex.cpp

130

You're not passing this as a const, so the calling function's Cursor will be updated - this doesn't sound generally safe to me - what about a move?

IIUC, GlobalModuleIndex::GlobalModuleIndex is a private function that has only been called by GlobalModuleIndex::readIndex, and the Cursor object has only one consumer (GlobalModuleIndex::readIndex), so I guess there should not be any unsafe concern.

[dblaikie]

Perhaps GlobalModuleIndex should create the cursor itself - it's being handed the buffer anyway?

[junaire]

use std::move().

I'm not sure this will be better but hope it will make others happy :)

[RKSimon]

LGTM

[dblaikie]

In D123436#3462567, @dblaikie wrote:

Perhaps GlobalModuleIndex should create the cursor itself - it's being handed the buffer anyway?

Ping on this ^ - would this be a better direction that addresses the concerns?

[junaire]

In D123436#3475002, @dblaikie wrote:

In D123436#3462567, @dblaikie wrote:

Perhaps GlobalModuleIndex should create the cursor itself - it's being handed the buffer anyway?

Ping on this ^ - would this be a better direction that addresses the concerns?

Sorry about missing this! :(
However, I'm not sure that I understand your idea. Do you mean we can simply pass the buffer to the GlobalModuleIndex then we can construct the cursor in the constructor itself? IMHO, we can't. that's because we need to use the cursor to sniff for the signature of the buffer, and we will return an error if it is failed. Please let me know if I understand you wrong :)

  /// The main bitstream cursor for the main block.
  llvm::BitstreamCursor Cursor(*Buffer);

  // Sniff for the signature.
  for (unsigned char C : {'B', 'C', 'G', 'I'}) {
    if (Expected<llvm::SimpleBitstreamCursor::word_t> Res = Cursor.Read(8)) {
      if (Res.get() != C)
        return std::make_pair(
            nullptr, llvm::createStringError(std::errc::illegal_byte_sequence,
                                             "expected signature BCGI"));
    } else
      return std::make_pair(nullptr, Res.takeError());
  }

  return std::make_pair(new GlobalModuleIndex(std::move(Buffer), std::move(Cursor)),
                        llvm::Error::success());
}

[dblaikie]

In D123436#3475858, @junaire wrote:

In D123436#3475002, @dblaikie wrote:

In D123436#3462567, @dblaikie wrote:

Perhaps GlobalModuleIndex should create the cursor itself - it's being handed the buffer anyway?

Ping on this ^ - would this be a better direction that addresses the concerns?

Sorry about missing this! :(
However, I'm not sure that I understand your idea. Do you mean we can simply pass the buffer to the GlobalModuleIndex then we can construct the cursor in the constructor itself?

That's what I meant, yeah - since the buffer is actually already being passed.

IMHO, we can't. that's because we need to use the cursor to sniff for the signature of the buffer, and we will return an error if it is failed. Please let me know if I understand you wrong :)

Ah, right ,I missed that usage before the call. Makes sense then - thanks for helping me understand!

clang/lib/Serialization/GlobalModuleIndex.cpp

 
 typedef llvm::OnDiskIterableChainedHashTable<IdentifierIndexReaderTrait>
     IdentifierIndexTable;
 
 }
 
 GlobalModuleIndex::GlobalModuleIndex(
     std::unique_ptr<llvm::MemoryBuffer> IndexBuffer,
     llvm::BitstreamCursor Cursor)
     : Buffer(std::move(IndexBuffer)), IdentifierIndex(), NumIdentifierLookups(),
       NumIdentifierLookupHits() {
   auto Fail = [&](llvm::Error &&Err) {
     report_fatal_error("Module index '" + Buffer->getBufferIdentifier() +
                        "' failed: " + toString(std::move(Err)));
   };
 
   llvm::TimeTraceScope TimeScope("Module LoadIndex");
if (Expected<llvm::SimpleBitstreamCursor::word_t> Res = Cursor.Read(8)) {
       if (Res.get() != C)
         return std::make_pair(
             nullptr, llvm::createStringError(std::errc::illegal_byte_sequence,
                                              "expected signature BCGI"));
     } else
       return std::make_pair(nullptr, Res.takeError());
   }
 
-  return std::make_pair(new GlobalModuleIndex(std::move(Buffer), Cursor),
+  return std::make_pair(new GlobalModuleIndex(std::move(Buffer), std::move(Cursor)),
                         llvm::Error::success());
 }
 
 void
 GlobalModuleIndex::getKnownModules(SmallVectorImpl<ModuleFile *> &ModuleFiles) {
   ModuleFiles.clear();
   for (unsigned I = 0, N = Modules.size(); I != N; ++I) {
     if (ModuleFile *MF = Modules[I].File)