[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/include/clang/Serialization/GlobalModuleIndex.h

class GlobalModuleIndex {
   unsigned NumIdentifierLookups;
 
   /// The number of identifier lookup hits, where we recognize the
   /// identifier.
   unsigned NumIdentifierLookupHits;
 
   /// Internal constructor. Use \c readIndex() to read an index.
   explicit GlobalModuleIndex(std::unique_ptr<llvm::MemoryBuffer> Buffer,
-                             llvm::BitstreamCursor Cursor);
+                             llvm::BitstreamCursor &Cursor);
 
   GlobalModuleIndex(const GlobalModuleIndex &) = delete;
   GlobalModuleIndex &operator=(const GlobalModuleIndex &) = delete;
 
 public:
   ~GlobalModuleIndex();
 
   /// Read a global index file for the given directory.

clang/lib/Serialization/GlobalModuleIndex.cpp

 
 typedef llvm::OnDiskIterableChainedHashTable<IdentifierIndexReaderTrait>
     IdentifierIndexTable;
 
 }
 
 GlobalModuleIndex::GlobalModuleIndex(
     std::unique_ptr<llvm::MemoryBuffer> IndexBuffer,
-    llvm::BitstreamCursor Cursor)
+    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");