Frontend: Take VFS and MainFileBuffer by reference in PrecompiledPreamble::CanReuse, NFC

[dexonsmith]

Summary

Clarify that PrecompiledPreamble::CanReuse requires non-null arguments
for VFS and MainFileBuffer, taking them by reference instead of by
pointer.

[dexonsmith]

ping

[kadircet]

clang/include/clang/Frontend/PrecompiledPreamble.h

108–109

why not accept a value directly here? (i.e. drop const and ref)

[dexonsmith]

clang/include/clang/Frontend/PrecompiledPreamble.h

108–109

Ah, yes, I've done this a few times, and it still seems not quite right. But the alternative also doesn't feel right when it's not necessary:

#include "llvm/Basic/MemoryBufferRef.h"

I'm happy either way since that file is fairly careful to avoid bloating includes.

[jansvoboda11]

LGTM

clang/include/clang/Frontend/PrecompiledPreamble.h

108–109

I agree this looks a bit odd, but avoiding an unnecessary include seems like a good excuse.

[dexonsmith]

clang/include/clang/Frontend/PrecompiledPreamble.h

108–109

@kadircet , WDYT?

[dexonsmith]

@kadircet, ping, do you still have concerns?

[dexonsmith]

@kadircet, adding you explicitly as a blocking reviewer, since I suspect you've missed any emails generated from my previous pings. Please let me know whether you still have concerns.

[kadircet]

clang/include/clang/Frontend/PrecompiledPreamble.h

108–109

sorry i was on vacation and just got the chance to get back to this.

I don't feel so bad about the include, as the header itself is small-ish and only includes StringRef.h, which is already included by this header. So I would lean towards accepting this by value and keeping the API clean, rather than trying to get away with a forward declaration.

but definitely up to you, i don't feel strongly about it in any case. (as you can easily make the argument of header for MemoryBufferRef getting bloated over time)

[dexonsmith]

Thanks for the reviews; pushed f4d02fbe418db55375b78b8f57e47126e4642fb6.

clang/include/clang/Frontend/PrecompiledPreamble.h

108–109

I decided to leave it as-is for now to match the other MemoryBufferRef APIs in the file... but I take your point that MemoryBufferRef is a cheap include and I expect it will stay cheap. I'll aim to circle back and update the whole file at some point.

clang-tools-extra/clangd/Preamble.cpp

bool isPreambleCompatible(const PreambleData &Preamble,
                           const ParseInputs &Inputs, PathRef FileName,
                           const CompilerInvocation &CI) {
   auto ContentsBuffer =
       llvm::MemoryBuffer::getMemBuffer(Inputs.Contents, FileName);
   auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), *ContentsBuffer, 0);
   auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory);
   return compileCommandsAreEqual(Inputs.CompileCommand,
                                  Preamble.CompileCommand) &&
-         Preamble.Preamble.CanReuse(CI, ContentsBuffer.get(), Bounds,
-                                    VFS.get());
+         Preamble.Preamble.CanReuse(CI, *ContentsBuffer, Bounds, *VFS);
 }
 
 void escapeBackslashAndQuotes(llvm::StringRef Text, llvm::raw_ostream &OS) {
   for (char C : Text) {
     switch (C) {
     case '\\':
     case '"':
       OS << '\\';

clang/include/clang/Frontend/PrecompiledPreamble.h

public:
   /// Returned string is not null-terminated.
   llvm::StringRef getContents() const {
     return {PreambleBytes.data(), PreambleBytes.size()};
   }
 
   /// Check whether PrecompiledPreamble can be reused for the new contents(\p
   /// MainFileBuffer) of the main file.
   bool CanReuse(const CompilerInvocation &Invocation,
-                const llvm::MemoryBuffer *MainFileBuffer, PreambleBounds Bounds,
-                llvm::vfs::FileSystem *VFS) const;
+                const llvm::MemoryBufferRef &MainFileBuffer,
+                PreambleBounds Bounds, llvm::vfs::FileSystem &VFS) const;
 
   /// Changes options inside \p CI to use PCH from this preamble. Also remaps
   /// main file to \p MainFileBuffer and updates \p VFS to ensure the preamble
   /// is accessible.
   /// Requires that CanReuse() is true.
   /// For in-memory preambles, PrecompiledPreamble instance continues to own the
   /// MemoryBuffer with the Preamble after this method returns. The caller is
   /// responsible for making sure the PrecompiledPreamble instance outlives the

clang/lib/Frontend/ASTUnit.cpp

if (!MainFileBuffer)
     return nullptr;
 
   PreambleBounds Bounds = ComputePreambleBounds(
       *PreambleInvocationIn.getLangOpts(), *MainFileBuffer, MaxLines);
   if (!Bounds.Size)
     return nullptr;
 
   if (Preamble) {
-    if (Preamble->CanReuse(PreambleInvocationIn, MainFileBuffer.get(), Bounds,
-                           VFS.get())) {
+    if (Preamble->CanReuse(PreambleInvocationIn, *MainFileBuffer, Bounds,
+                           *VFS)) {
       // Okay! We can re-use the precompiled preamble.
 
       // Set the state of the diagnostic object to mimic its state
       // after parsing the preamble.
       getDiagnostics().Reset();
       ProcessWarningOptions(getDiagnostics(),
                             PreambleInvocationIn.getDiagnosticOpts());
       getDiagnostics().setNumWarnings(NumWarningsInPreamble);

clang/lib/Frontend/PrecompiledPreamble.cpp

assert(Result <= std::numeric_limits<std::size_t>::max() &&
            "file size did not fit into size_t");
     return Result;
   }
   }
   llvm_unreachable("Unhandled storage kind");
 }
 
 bool PrecompiledPreamble::CanReuse(const CompilerInvocation &Invocation,
-                                   const llvm::MemoryBuffer *MainFileBuffer,
+                                   const llvm::MemoryBufferRef &MainFileBuffer,
                                    PreambleBounds Bounds,
-                                   llvm::vfs::FileSystem *VFS) const {
+                                   llvm::vfs::FileSystem &VFS) const {
 
   assert(
-      Bounds.Size <= MainFileBuffer->getBufferSize() &&
+      Bounds.Size <= MainFileBuffer.getBufferSize() &&
       "Buffer is too large. Bounds were calculated from a different buffer?");
 
   auto PreambleInvocation = std::make_shared<CompilerInvocation>(Invocation);
   PreprocessorOptions &PreprocessorOpts =
       PreambleInvocation->getPreprocessorOpts();
 
   // We've previously computed a preamble. Check whether we have the same
   // preamble now that we did before, and that there's enough space in
   // the main-file buffer within the precompiled preamble to fit the
   // new main file.
   if (PreambleBytes.size() != Bounds.Size ||
       PreambleEndsAtStartOfLine != Bounds.PreambleEndsAtStartOfLine ||
       !std::equal(PreambleBytes.begin(), PreambleBytes.end(),
-                  MainFileBuffer->getBuffer().begin()))
+                  MainFileBuffer.getBuffer().begin()))
     return false;
   // The preamble has not changed. We may be able to re-use the precompiled
   // preamble.
 
   // Check that none of the files used by the preamble have changed.
   // First, make a record of those files that have been overridden via
   // remapping or unsaved_files.
   std::map<llvm::sys::fs::UniqueID, PreambleFileHash> OverriddenFiles;
   llvm::StringSet<> OverriddenAbsPaths; // Either by buffers or files.
   for (const auto &R : PreprocessorOpts.RemappedFiles) {
     llvm::vfs::Status Status;
-    if (!moveOnNoError(VFS->status(R.second), Status)) {
+    if (!moveOnNoError(VFS.status(R.second), Status)) {
       // If we can't stat the file we're remapping to, assume that something
       // horrible happened.
       return false;
     }
     // If a mapped file was previously missing, then it has changed.
     llvm::SmallString<128> MappedPath(R.first);
-    if (!VFS->makeAbsolute(MappedPath))
+    if (!VFS.makeAbsolute(MappedPath))
       OverriddenAbsPaths.insert(MappedPath);
 
     OverriddenFiles[Status.getUniqueID()] = PreambleFileHash::createForFile(
         Status.getSize(), llvm::sys::toTimeT(Status.getLastModificationTime()));
   }
 
   // OverridenFileBuffers tracks only the files not found in VFS.
   llvm::StringMap<PreambleFileHash> OverridenFileBuffers;
   for (const auto &RB : PreprocessorOpts.RemappedFileBuffers) {
     const PrecompiledPreamble::PreambleFileHash PreambleHash =
         PreambleFileHash::createForMemoryBuffer(RB.second->getMemBufferRef());
     llvm::vfs::Status Status;
-    if (moveOnNoError(VFS->status(RB.first), Status))
+    if (moveOnNoError(VFS.status(RB.first), Status))
       OverriddenFiles[Status.getUniqueID()] = PreambleHash;
     else
       OverridenFileBuffers[RB.first] = PreambleHash;
 
     llvm::SmallString<128> MappedPath(RB.first);
-    if (!VFS->makeAbsolute(MappedPath))
+    if (!VFS.makeAbsolute(MappedPath))
       OverriddenAbsPaths.insert(MappedPath);
   }
 
   // Check whether anything has changed.
   for (const auto &F : FilesInPreamble) {
     auto OverridenFileBuffer = OverridenFileBuffers.find(F.first());
     if (OverridenFileBuffer != OverridenFileBuffers.end()) {
       // The file's buffer was remapped and the file was not found in VFS.
       // Check whether it matches up with the previous mapping.
       if (OverridenFileBuffer->second != F.second)
         return false;
       continue;
     }
 
     llvm::vfs::Status Status;
-    if (!moveOnNoError(VFS->status(F.first()), Status)) {
+    if (!moveOnNoError(VFS.status(F.first()), Status)) {
       // If the file's buffer is not remapped and we can't stat it,
       // assume that something horrible happened.
       return false;
     }
 
     std::map<llvm::sys::fs::UniqueID, PreambleFileHash>::iterator Overridden =
         OverriddenFiles.find(Status.getUniqueID());
     if (Overridden != OverriddenFiles.end()) {
if (Status.getSize() != uint64_t(F.second.Size) ||
       return false;
   }
   for (const auto &F : MissingFiles) {
     // A missing file may be "provided" by an override buffer or file.
     if (OverriddenAbsPaths.count(F.getKey()))
       return false;
     // If a file previously recorded as missing exists as a regular file, then
     // consider the preamble out-of-date.
-    if (auto Status = VFS->status(F.getKey())) {
+    if (auto Status = VFS.status(F.getKey())) {
       if (Status->isRegularFile())
         return false;
     }
   }
   return true;
 }
 
 void PrecompiledPreamble::AddImplicitPreamble(