This is an archive of the discontinued LLVM Phabricator instance.

FileManager: std::map => BumpPtrAllocator + DenseMap of pointers
ClosedPublic

Authored by sammccall on Apr 5 2022, 10:07 AM.

Details

Diff Detail

Event Timeline

sammccall created this revision.Apr 5 2022, 10:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2022, 10:07 AM
Herald added a subscriber: dexonsmith. · View Herald Transcript
sammccall requested review of this revision.Apr 5 2022, 10:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2022, 10:07 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
kadircet accepted this revision.Apr 5 2022, 10:46 AM

thanks! maybe put an NFC in the patch name?

clang/lib/Basic/FileManager.cpp
581

seems like just formatting change, same for next 2 chunks.

This revision is now accepted and ready to land.Apr 5 2022, 10:46 AM
This revision was landed with ongoing or failed builds.Apr 5 2022, 10:55 AM
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
dexonsmith added inline comments.Apr 5 2022, 6:37 PM
clang/lib/Basic/FileManager.cpp
318

I'm trying to figure out why ReusingEntry was necessary. It looks to me like you could just call UFE->isValid() here and avoid introducing that bool... or am I missing something?

sammccall added inline comments.Apr 6 2022, 1:11 AM
clang/lib/Basic/FileManager.cpp
318

You're right, but I think we can get rid of isValid, and plan to send a patch.

A FileEntry handed out by FileManager always has isValid = true. It's not possible to create a useful FileEntry outside FileManager (and this is never done outside FileManagerTest). So the concept of validity isn't needed.

(IMO the main cost of having the isValid flag is conceptual complexity - lots of places check it, which means people must be thinking about how to handle this case)

dexonsmith added inline comments.
clang/lib/Basic/FileManager.cpp
318

Sure, SGTM.