[PATCH] D92136: [clang] Enhanced test for --relocatable-pch, and corresponding fixes for Windows

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Seems reasonable



================
Comment at: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp:534
 
+#ifdef _WIN32
+        // Support special case of Windows absolute file paths
----------------
rnk wrote:
> I think looking for a slash after the colon is a bit more robust, and worth doing. `[a-zA-Z]:[/\\]`
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.


================
Comment at: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp:534-537
+#ifdef _WIN32
+        // Support special case of Windows absolute file paths
+        if (Filename.size() == 1 && isLetter(Filename.front()) &&
+            PH.Search(":")) {
----------------
I think looking for a slash after the colon is a bit more robust, and worth doing. `[a-zA-Z]:[/\\]`


================
Comment at: llvm/unittests/Support/Path.cpp:1477
 
+TEST(Support, StartsWith) {
+  EXPECT_FALSE(starts_with("/foo/bar", "/bar", path::Style::posix));
----------------
Thank you for adding unit tests!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92136/new/

https://reviews.llvm.org/D92136