Fix for pr31836 - pp_nonportable_path on absolute paths: broken delimiters

[twoh]

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.

[eric_niebler]

lib/Lex/PPDirectives.cpp

1983

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

@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]

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]

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]

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

[karies]

lib/Lex/PPDirectives.cpp

1983

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]

test/Lexer/case-insensitive-include-pr31836.sh

7

@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]

test/Lexer/case-insensitive-include-pr31836.sh

7

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

[twoh]

@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]

Nope looks good.

lib/Lex/PPDirectives.cpp

if (CheckIncludePathPortability) {
     StringRef RealPathName = File->tryGetRealPathName();
     SmallVector<StringRef, 16> Components(llvm::sys::path::begin(Name),
                                           llvm::sys::path::end(Name));
 
     if (trySimplifyPath(Components, RealPathName)) {
       SmallString<128> Path;
       Path.reserve(Name.size()+2);
       Path.push_back(isAngled ? '<' : '"');
+      bool isLeadingSeparator = llvm::sys::path::is_absolute(Name);
       for (auto Component : Components) {
+        if (isLeadingSeparator)
+          isLeadingSeparator = false;
+        else
           Path.append(Component);
         // Append the separator the user used, or the close quote
         Path.push_back(
           Path.size() <= Filename.size() ? Filename[Path.size()-1] :
             (isAngled ? '>' : '"'));
       }
       // For user files and known standard headers, by default we issue a diagnostic.
       // For other system headers, we don't. They can be controlled separately.
       auto DiagId = (FileCharacter == SrcMgr::C_User || warnByDefaultOnWrongCase(Name)) ?

test/Lexer/case-insensitive-include-pr31836.sh

+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T
+// RUN: touch %T/case-insensitive-include-pr31836.h
+// RUN: echo "#include \"%T/Case-Insensitive-Include-Pr31836.h\"" | %clang_cc1 -E - 2>&1 | FileCheck %s
+
+// CHECK: warning: non-portable path to file
+// CHECK-NOT: Ccase-insensitive-include-pr31836.h
+// CHECK: {{$}}