This is an archive of the discontinued LLVM Phabricator instance.

Split out llvm/Support/FileSystem/UniqueID.h and clang/Basic/FileEntry.h, NFC
ClosedPublic

Authored by dexonsmith on Oct 19 2020, 8:00 PM.

Details

Summary

Split out llvm/Support/FileSystem/UniqueID.h and clang/Basic/FileEntry.h, NFC

Split FileEntry and FileEntryRef out into a new file
clang/Basic/FileEntry.h. This allows current users of a
forward-declared FileEntry to transition to FileEntryRef without
adding more includers of FileManager.h.

Also split UniqueID out to llvm/Support/FileSystem/UniqueID.h, so
FileEntry.h doesn't need to include all of FileSystem.h for just
that type.

Diff Detail

Event Timeline

dexonsmith created this revision.Oct 19 2020, 8:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 19 2020, 8:00 PM
JDevlieghere added inline comments.Oct 19 2020, 10:20 PM
clang/include/clang/Basic/FileEntry.h
34

Won't this now make llvm::Optional visible as clang::Optional everywhere this header is included? Isn't this considered bad practice in a header?

dexonsmith added inline comments.Oct 20 2020, 5:47 AM
clang/include/clang/Basic/FileEntry.h
34

I thought I found both of these usable in clang headers elsewhere. I’ll double check.

dexonsmith added inline comments.Oct 20 2020, 8:19 AM
clang/include/clang/Basic/FileEntry.h
34

Ah, I was sort of right. The correct way to do this is:

#include <clang/Basic/LLVM.h>

which forward-declares a bunch of stuff and pulls it into clang::.

I'll update.

Use #include <clang/Basic/LLVM.h> to pull StringRef and Optional into clang::.

JDevlieghere accepted this revision.Oct 23 2020, 8:46 PM
This revision is now accepted and ready to land.Oct 23 2020, 8:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2020, 1:38 PM
rnk added a subscriber: rnk.Oct 29 2020, 3:50 PM

I just saw this in passing. Thanks a lot, I had noticed that Filesystem.h is super expensive.

llvm/include/llvm/Support/FileSystem/UniqueID.h
36

FYI std::tie is pretty expensive to compile, unfortunately. You also might technically need to include some STL header for it here.

dexonsmith added inline comments.Oct 30 2020, 8:41 AM
llvm/include/llvm/Support/FileSystem/UniqueID.h
36

I just sent you https://reviews.llvm.org/D90471 to remove std::tie. (Already fixed the includes in 44d65efd95b353eeb26440ad2ef9f3bd05f5a927, not sure how/why it worked without them locally.)