This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by dexonsmith on Nov 11 2020, 1:26 PM.

Details

Summary

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

Diff Detail

Event Timeline

dexonsmith created this revision.Nov 11 2020, 1:26 PM
dexonsmith requested review of this revision.Nov 11 2020, 1:26 PM
kadircet added inline comments.Nov 19 2020, 12:22 AM
clang/include/clang/Frontend/PrecompiledPreamble.h
108

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

dexonsmith added inline comments.Nov 19 2020, 6:28 PM
clang/include/clang/Frontend/PrecompiledPreamble.h
108

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 accepted this revision.Dec 1 2020, 8:10 AM

LGTM

clang/include/clang/Frontend/PrecompiledPreamble.h
108

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

This revision is now accepted and ready to land.Dec 1 2020, 8:10 AM
dexonsmith added inline comments.Dec 1 2020, 4:26 PM
clang/include/clang/Frontend/PrecompiledPreamble.h
108

@kadircet , WDYT?

@kadircet, ping, do you still have concerns?

@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.

This revision now requires review to proceed.Dec 14 2020, 12:12 PM
kadircet accepted this revision.Jan 8 2021, 4:26 AM
kadircet added inline comments.
clang/include/clang/Frontend/PrecompiledPreamble.h
108

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)

This revision is now accepted and ready to land.Jan 8 2021, 4:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2021, 5:51 PM

Thanks for the reviews; pushed f4d02fbe418db55375b78b8f57e47126e4642fb6.

clang/include/clang/Frontend/PrecompiledPreamble.h
108

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.