This is an archive of the discontinued LLVM Phabricator instance.

[clang] use FileEntryRef for SUBMODULE_TOPHEADER
AbandonedPublic

Authored by rmaz on Jan 27 2023, 7:31 AM.

Details

Summary

Refactor a Module's TopHeaders to use FileEntryRef. This will keep
the paths serialized in a module the same as the ones used to look
up the header initially.

Diff Detail

Event Timeline

rmaz created this revision.Jan 27 2023, 7:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2023, 7:31 AM
Herald added a subscriber: arphaman. · View Herald Transcript
rmaz requested review of this revision.Jan 27 2023, 7:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2023, 7:31 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jansvoboda11 added inline comments.Jan 27 2023, 9:16 AM
clang/lib/Basic/Module.cpp
271–272

If we assert that File is not empty right away, why take OptionalFileEntryRef instead of FileEntryRef?

clang/lib/Lex/ModuleMap.cpp
663

How much work would be to refactor File to be FileEntryRef? Would be good if we didn't need to resort to getLastRef().

rmaz updated this revision to Diff 492829.Jan 27 2023, 9:55 AM

Use FileEntryRef for AddTopHeader()

rmaz marked an inline comment as done.Jan 27 2023, 10:07 AM
rmaz added inline comments.
clang/lib/Lex/ModuleMap.cpp
663

It is unfortunately a fair bit. I spent some time on it but hit a roadblock with the FileManagers VirtualFileEntries:

https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/FileManager.h#L70-L71

We would require this to be refactored to FileEntry too, as it is used here to provide a UID mapping to FileEntry pointers which needs to be changed to refs:

https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/FileManager.cpp#L614-L633

I can put up a follow on diff once this one is shipped if preferred?

jansvoboda11 added inline comments.Jan 27 2023, 12:08 PM
clang/lib/Lex/ModuleMap.cpp
663

I was able to refactor the UID mapping to use FileEntryRef in D142780. Given that, I think it would be preferable to propagate these into findOrCreateModuleForHeaderInUmbrellaDir() and remove getLastRef().

rmaz added inline comments.Jan 27 2023, 12:21 PM
clang/lib/Lex/ModuleMap.cpp
663

Nice, i'll rebase this and add the remaining changes once its shipped.

rmaz abandoned this revision.Apr 11 2023, 7:15 AM