kadircet accepted this revision. kadircet added inline comments. This revision is now accepted and ready to land.
================ 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; ---------------- 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) 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
