Page MenuHomePhabricator

[clang] Enhanced test for --relocatable-pch, and corresponding fixes for Windows
Needs ReviewPublic

Authored by saudi on Nov 25 2020, 1:57 PM.

Details

Summary

The test now moves the header files between the pch creation and its use.
This change caused failures under Windows:

  • The -isysroot path matching was failing when given variations of casing or separators (/ vs \)
  • clang -verify checks "// expected-[note/warning/...]@[file]:[line] ..." would fail when file is an absolute path, since the : character was interpreted as the delimiter between [file] and [line].

Diff Detail

Event Timeline

saudi created this revision.Nov 25 2020, 1:57 PM
saudi requested review of this revision.Nov 25 2020, 1:57 PM
saudi updated this revision to Diff 307854.Nov 26 2020, 6:29 AM

Update patch : clang-format fixes.

There remains some clang-tidy warnings in Path.cpp and Path.h from llvm/Support.
I would like extra opinions on this, because fixing them would break the homogeneity with the rest of the file, which exposes snake_case - like function names.
More code from llvm/Support have that clang-tidy mismatch, but adding Support to the clang-tidy exclusion seems a bit extreme...

Bit out of my wheelhouse hopefully @rsmith or @rnk could chime in at some point.

rnk added a comment.Jan 21 2021, 1:30 PM

Seems reasonable

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
534

I'm trying to imagine a use case where the user might be validating diagnostics generated with a Windows style virtual filesystem, but I can't think of how that could happen.

In any case, I think we should drop the ifdef, and just do the special case on all platforms, especially if you tighten it up with the slash check.

534–537

I think looking for a slash after the colon is a bit more robust, and worth doing. [a-zA-Z]:[/\\]

llvm/unittests/Support/Path.cpp
1477

Thank you for adding unit tests!