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

[Marek Kurdej via cfe-commits]

curdeius added a subscriber: curdeius.
curdeius added a comment.

Some minor remarks. Sorry for being finicky :).


================
Comment at: include/clang/Basic/VirtualFileSystem.h:97
@@ +96,3 @@
+      return Status->getName();
+    else
+      return Status.getError();
----------------
No else needed after return.

================
Comment at: lib/Lex/PPDirectives.cpp:1673
@@ +1672,3 @@
+  if (File && !File->tryGetRealPathName().empty() &&
+    !Diags->isIgnored(diag::pp_nonportable_path, FilenameTok.getLocation())) {
+    StringRef Name = LangOpts.MSVCCompat ? NormalizedPath.c_str() : Filename;
----------------
May you create a bool variable for this long condition and name it so that it's clear what it checks?

================
Comment at: lib/Lex/PPDirectives.cpp:1678-1700
@@ +1677,25 @@
+                                          llvm::sys::path::end(Name));
+    auto iRealPathComponent = llvm::sys::path::rbegin(RealPathName);
+    auto iRealPathComponentEnd = llvm::sys::path::rend(RealPathName);
+    int Cnt = 0;
+    bool SuggestReplacement = false;
+    for(auto& Component : llvm::reverse(Components)) {
+      if ("." == Component) {
+      } else if (".." == Component) {
+        ++Cnt;
+      } else if (Cnt) {
+        --Cnt;
+      } else if (iRealPathComponent != iRealPathComponentEnd) {
+        if (Component != *iRealPathComponent) {
+          // If these path components differ by more than just case, then we
+          // may be looking at symlinked paths. Bail on this diagnostic to avoid
+          // noisy false positives.
+          SuggestReplacement = iRealPathComponent->equals_lower(Component);
+          if (!SuggestReplacement)
+            break;
+          Component = *iRealPathComponent;
+        }
+        ++iRealPathComponent;
+      }
+    }
+    if (SuggestReplacement) {
----------------
Could you extract a function for this chunk? Something like `bool SuggestReplacement = simplifyPath(Components);`.
The `HandleIncludeDirective` method is already loooong enough.


http://reviews.llvm.org/D19843