This is an archive of the discontinued LLVM Phabricator instance.

[clang][lex] NFCI: Use DirectoryEntryRef in HeaderSearch::LookupFile
ClosedPublic

Authored by jansvoboda11 on Jun 13 2022, 8:49 AM.

Details

Summary

This patch changes the argument type to HeaderSearch::LookupFile() from const DirectoryEntry * to DirectoryEntryRef in order to remove some calls to the deprecated DirectoryEntry::getName().

Depends on D127660.

Diff Detail

Event Timeline

jansvoboda11 created this revision.Jun 13 2022, 8:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2022, 8:49 AM
jansvoboda11 requested review of this revision.Jun 13 2022, 8:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2022, 8:49 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
bnbarham accepted this revision.Jun 13 2022, 11:15 AM
bnbarham added inline comments.
clang/lib/Frontend/FrontendAction.cpp
828–830

Worth adding an assert here? It'd be nice to cleanup the CWD handling in FileManager at some point.

clang/lib/Lex/PPDirectives.cpp
938

Could we change this to a FileEntryRef as well, or would you prefer to do that later? It'll touch basically all the same places again (eg. each parameter above).

This revision is now accepted and ready to land.Jun 13 2022, 11:15 AM

Failing test seems to be because . is turned into the full path at some point. It's possible that this is because getFileRef returns the absolute path (see the massive FIXME there). If that's the case we could fix that by fixing clang-apply-replacements and then change the Status.getName() == Filename to only check whether it's an externally mapped path.

Fix the Modules/filename.cpp test

The failing test was a fun one to debug. It didn't test what it intended to. It wanted to check that header referenced from module map is found via a search path, but the command line was constructed in such way that the header was found relative to the includer. The test relied on the FileManager hackery that mutates FileEntry objects. Now that we call SourceMgr.getFileEntryRefForID(), we don't see effects of this hack. I ended up only fixing the test, since removing the FileManager hack safely probably requires us to remove all remaining {File,Directory}Entry::getName() usages.

LMK if you have more feedback.

clang/lib/Frontend/FrontendAction.cpp
828–830

Assert that Dir is not empty? Seems overly defensive to me, what would be the benefit of that? Catching if the working directory was yanked from underneath Clang?

clang/lib/Lex/PPDirectives.cpp
938

I'd prefer to do it separately for smaller diff and easier bisection. LMK if you have strong preference for doing it in the same commit.

Match Windows \\ path separators too

bnbarham accepted this revision.May 30 2023, 5:21 PM
bnbarham added inline comments.
clang/lib/Frontend/FrontendAction.cpp
828–830

Not sure what I was thinking here - the assertion itself would be pointless, since *Dir will assert non-empty anyway. I imagine this was more about how we're grabbing an optional and not checking it, which IMO is often an anti pattern. What do you want to happen if it is somehow empty? Couldn't we just not do the push_back?

Also could use emplace_back(nullptr, *Dir) instead, not that it really matters.

clang/lib/Lex/PPDirectives.cpp
938

Not really, just seemed like it would hit all the same lines to me so it'd be roughly the same in terms of diff size. Up to you.

clang/test/Modules/filename.cpp
2

So much nicer with split-file, thanks for updating!

benlangmuir accepted this revision.May 30 2023, 5:49 PM
This revision was landed with ongoing or failed builds.May 30 2023, 9:42 PM
This revision was automatically updated to reflect the committed changes.
clang/lib/Lex/PPDirectives.cpp