Page MenuHomePhabricator

Make -Wnonportable-include-path ignore drive case on Windows.
ClosedPublic

Authored by thakis on Wed, May 6, 5:24 PM.

Details

Summary

See PR45812 for motivation.

No explicit test since I couldn't figure out how to get the
current disk drive in lower case into a form in lit where I could
mkdir it and cd to it. But the change does have test coverage in
that I can remove the case normalization in lit, and tests failed
on several bots (and for me locally if in a pwd with a lower-case
drive) without that normalization prior to this change.

Diff Detail

Event Timeline

thakis created this revision.Wed, May 6, 5:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, May 6, 5:24 PM
rnk added a subscriber: rnk.Wed, May 6, 5:53 PM
rnk added inline comments.
clang/lib/Lex/PPDirectives.cpp
2118

It seems like trySimplifyPath could check if it is checking the case of the first component on Windows, and that would hide the complexity from the main Preprocessor::HandleHeaderIncludeOrImport implementation. It also saves a string copy for every include.

thakis marked an inline comment as done.Wed, May 6, 7:19 PM
thakis added inline comments.
clang/lib/Lex/PPDirectives.cpp
2118

Not easily, since that walks the path backwards. Includes are almost never absolute, so this also almost never does a string copy and it's imho a bit simpler this way.

I agree pulling out the code for this warning into its own function is a good idea though.

hans accepted this revision.Thu, May 7, 7:32 AM

Looks reasonable to me, but Windows paths are scary..

clang/lib/Lex/PPDirectives.cpp
2112

Maybe expand "was used" to "was used in the shell" or something. It may not be clear for people without context, since this whole thing is pretty obscure.

2123

Could it be different for e.g. network drives? I guess maybe they'd still have at least 3 components, but perhaps no drive letter

This revision is now accepted and ready to land.Thu, May 7, 7:32 AM
thakis marked an inline comment as done.Thu, May 7, 11:08 AM
thakis added inline comments.
clang/lib/Lex/PPDirectives.cpp
2123

Oh, good call, /FI\\?\%cd%\test.h produces a path that's is_absolute() but that returns

\\?
\
c:
\
src
llvm-project
test.h

as path components (one per line). Looking at how to handle that now. If anyone happens to know, please shout :)

thakis closed this revision.Thu, May 7, 12:55 PM
thakis marked an inline comment as done.

Landed in d03838343f2199580. Found another bug elsewhere while looking at this, will make a patch for that now.

clang/lib/Lex/PPDirectives.cpp
2123

This now strips the common UNC path at the start and restores it at the end if it was there, and there's a test for this.