[PATCH] D91297: Frontend: Take VFS and MainFileBuffer by reference in PrecompiledPreamble::CanReuse, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

Thanks for the reviews; pushed f4d02fbe418db55375b78b8f57e47126e4642fb6 <https://reviews.llvm.org/rGf4d02fbe418db55375b78b8f57e47126e4642fb6>.



================
Comment at: clang/include/clang/Frontend/PrecompiledPreamble.h:108
   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;
----------------
kadircet wrote:
> dexonsmith wrote:
> > jansvoboda11 wrote:
> > > dexonsmith wrote:
> > > > kadircet wrote:
> > > > > why not accept a value directly here? (i.e. drop const and ref)
> > > > 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.
> > > I agree this looks a bit odd, but avoiding an unnecessary include seems like a good excuse.
> > @kadircet , WDYT?
> 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)
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91297