[PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: lib/Lex/PPDirectives.cpp:33
@@ -28,2 +32,2 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SaveAndRestore.h"
----------------
eric_niebler wrote:
> You mean, instead of the `StringSet` below? Looks like `StringSwitch` just turns into cascading `memcmp`'s. Bet I can tell you how that performs versus a hash set. :-) Also, with the `StringSet`, I get to initialize it once and reuse it many times. I expect that will be pretty darn quick at runtime, but I'm looking forward to @bruno's results.
Right, I'm not suggesting `StringSwitch` will be faster; it's preferable for other reasons (it avoids the memory and shutdown costs of the static local set). We should stick with what you have if the performance advantage is measurable; otherwise my preference would be to use `StringSwitch`. But it's only a slight preference -- if you'd rather not, I won't complain.

[`StringSwitch` isn't /quite/ as bad as you're suggesting: it always first compares on length, and it typically compiles into a switch on string length followed by memcmps. Moreover, the code should be "obvious" enough that compilers can (at least in principle) optimize those memcmps very aggressively, right down into the equivalent of an unrolled DFA or a perfect hash function, but I'm not at all confident that LLVM will actually do that =)]

================
Comment at: lib/Lex/PPDirectives.cpp:220
@@ +219,3 @@
+    // In the ASCII range?
+    if (Ch < 0 || Ch > 0xff)
+      return false; // Can't be a standard header
----------------
Comment doesn't match code: the ASCII range ends at 0x7F.

================
Comment at: lib/Lex/PPDirectives.cpp:225-229
@@ +224,7 @@
+      Ch += 'a' - 'A';
+#ifdef LLVM_ON_WIN32
+    // Normalize path separators for comparison purposes.
+    else if (Ch == '\\')
+      Ch = '/';
+#endif
+  }
----------------
Rather than hardcoding this platform-specific test here, maybe use `llvm::sys::path::is_separator(Ch)`.


http://reviews.llvm.org/D19843