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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
