[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 a subscriber: rsmith.
rsmith added a comment.

Thanks for this! Sorry for the delay on the review side. Generally, the approach here looks fine, and I don't have any high-level concerns beyond a performance concern over whatever dark magic you're doing on the LLVM side to get this filename.

Please add some tests for the `FixItHint` replacement text. See test/FixIt for examples of how to do this.

I've left a handful of mostly-stylistic comments inline.


================
Comment at: include/clang/Basic/DiagnosticLexKinds.td:278
@@ +277,3 @@
+def pp_nonportable_path : Warning<
+  "Non-portable path '%0' found in preprocessor directive.">,
+  InGroup<NonportableIncludePath>;
----------------
Diagnostics should start with a lowercase letter and not end in a period.

================
Comment at: lib/Lex/PPDirectives.cpp:1503-1504
@@ -1499,1 +1502,4 @@
 
+namespace
+{
+  // Given a vector of path components and a string containing the real
----------------
`{` on previous line, please.

================
Comment at: lib/Lex/PPDirectives.cpp:1508
@@ +1507,3 @@
+  // and return true if the replacement should be suggested.
+  bool TrySimplifyPath(SmallVectorImpl<StringRef>& Components,
+                       StringRef RealPathName) {
----------------
`&` on the right.

================
Comment at: lib/Lex/PPDirectives.cpp:1510-1511
@@ +1509,4 @@
+                       StringRef RealPathName) {
+    auto iRealPathComponent = llvm::sys::path::rbegin(RealPathName);
+    auto iRealPathComponentEnd = llvm::sys::path::rend(RealPathName);
+    int Cnt = 0;
----------------
Local variable names should start with an uppercase letter.

================
Comment at: lib/Lex/PPDirectives.cpp:1514
@@ +1513,3 @@
+    bool SuggestReplacement = false;
+    for(auto& Component : llvm::reverse(Components)) {
+      if ("." == Component) {
----------------
`&` on the right.

================
Comment at: lib/Lex/PPDirectives.cpp:1516-1519
@@ +1515,6 @@
+      if ("." == Component) {
+      } else if (".." == Component) {
+        ++Cnt;
+      } else if (Cnt) {
+        --Cnt;
+      } else if (iRealPathComponent != iRealPathComponentEnd) {
----------------
Hmm. A `..` doesn't necessarily remove the prior path component, especially in the presence of directory symlinks. I suspect it's not worth the trouble of trying to get this right, but a comment saying that this is a best-effort attempt to handle `..`s would be useful.

================
Comment at: lib/Lex/PPDirectives.cpp:1704-1705
@@ +1703,4 @@
+  //
+  // Because testing for non-portable paths is expensive, only do it if the
+  // warning is not currently ignored.
+  const bool CheckIncludePathPortability =
----------------
Checking whether a warning is enabled can be surprisingly expensive; `TrySimplifyPath` might actually be cheaper.

================
Comment at: test/Lexer/case-insensitive-include.c:4
@@ +3,3 @@
+// RUN: mkdir -p %T/apath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -verify
----------------
Please explicitly `cd` to somewhere around `%T` here rather than assuming you know where it is relative to the current directory; we do not guarantee which directory the test will be run from and it does differ in practice between different ways of running the tests.

================
Comment at: test/Lexer/case-insensitive-include.c:8-11
@@ +7,6 @@
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: %clang_cc1 -fsyntax-only -fms-compatibility -DMS_COMPATIBILITY %s -include %s -I %T -verify
+
+#ifndef HEADER
+#define HEADER
+
----------------
What's the reason for this test including this file twice?


http://reviews.llvm.org/D19843