Page MenuHomePhabricator

Fix for pr31836 - pp_nonportable_path on absolute paths: broken delimiters
ClosedPublic

Authored by twoh on Feb 15 2017, 10:57 AM.

Details

Summary

This is a patch for PR31836. As the bug replaces the path separators in the included file name with the characters following them, the test script makes sure that there's no "Ccase-insensitive-include-pr31836.h" in the warning message.

Diff Detail

Repository
rL LLVM

Event Timeline

twoh created this revision.Feb 15 2017, 10:57 AM
eric_niebler added inline comments.
lib/Lex/PPDirectives.cpp
1983 ↗(On Diff #88580)

What happens on Windows for an absolute path like "C:/hello/world.h", I wonder? Does this correctly generate the fixit? @karies?

test/Lexer/case-insensitive-include-pr31836.sh
8 ↗(On Diff #88580)

@twoh I'm curious why you chose to check that the output was not a specific incorrect answer instead of checking that the output was the correct answer.

twoh updated this revision to Diff 88584.Feb 15 2017, 11:45 AM

Make it explicit that the test doesn't support windows. @eric_niebler, my original intention was avoiding use of platform-dependent path separator, but now made it explicit that the test is not for windows, it should be okay to use '/'. Thanks for the comments!

eric_niebler edited edge metadata.Feb 15 2017, 2:09 PM

My question was more about whether the code is correct for absolute paths on Windows, not about whether this particular test would pass or fail. Have you tested an incorrectly-cased absolute path on a Windows machine?

twoh added a comment.Feb 15 2017, 11:57 PM

@eric_niebler I just tried it on Windows machine, and it just succeeded with no warnings/fix-it. Is that expected?

karies added inline comments.Feb 16 2017, 12:11 AM
lib/Lex/PPDirectives.cpp
1983 ↗(On Diff #88580)

I also cannot reproduce this diag on Windows, likely because NTFS and FAT are case indifferent (not preserving like MacOS) so the file system cannot reply with "this is how *I* would have spelled the file name".

karies added inline comments.Feb 16 2017, 11:12 AM
test/Lexer/case-insensitive-include-pr31836.sh
6 ↗(On Diff #88584)

@twoh Does that actually work on Linux? I thought (most?) Linux file systems are case sensitive? I.e. I'd expect to get a "file not found" diag on the #include.

twoh added inline comments.Feb 16 2017, 11:16 AM
test/Lexer/case-insensitive-include-pr31836.sh
6 ↗(On Diff #88584)

This is an unsupported test on Linux because of REQUIRES: case-insensitive-filesystem.

twoh added a comment.Feb 21 2017, 11:43 AM

@eric_niebler Do you want any more experiments with this patch? I think Windows machines not printing warnings/fixits for absolute path is a separate issue with this.

eric_niebler accepted this revision.Feb 21 2017, 1:01 PM

Nope looks good.

This revision is now accepted and ready to land.Feb 21 2017, 1:01 PM
This revision was automatically updated to reflect the committed changes.