[PATCH] D19842: In openFileForRead, attempt to fetch the actual name of the file on disk -- including case -- so that clang can later warn about non-portable #include and #import directives.

[Bruno Cardoso Lopes via llvm-commits]

bruno added a comment.

Hi Eric,

LGTM with the comments below.


================
Comment at: lib/Support/Unix/Path.inc:553
@@ +552,3 @@
+namespace {
+  bool hasProcSelfFD() {
+    // If we have a /proc filesystem mounted, we ca quickly establish the
----------------
I would rather just have this logic inlined in the call site below.

================
Comment at: lib/Support/Unix/Path.inc:554
@@ +553,3 @@
+  bool hasProcSelfFD() {
+    // If we have a /proc filesystem mounted, we ca quickly establish the
+    // real name of the file with readlink
----------------
*can

================
Comment at: lib/Support/Unix/Path.inc:556
@@ +555,3 @@
+    // real name of the file with readlink
+    static const bool Result = (0 == ::access("/proc/self/fd", R_OK));
+    return Result;
----------------
It's more common in LLVM to use a integer literal in the RHS of the comparison, can you change this here and elsewhere in the patch? 

================
Comment at: lib/Support/Windows/Path.inc:831
@@ +830,3 @@
+  
+  if (0 == CharCount)
+    return mapWindowsError(::GetLastError());
----------------
Ditto

================
Comment at: lib/Support/Windows/Path.inc:837
@@ +836,3 @@
+  // On earlier Windows releases, the character count includes the terminating null.
+  if ('\0' == ResultPath.back())
+    ResultPath.pop_back();
----------------
Ditto

================
Comment at: unittests/Support/Path.cpp:999
@@ +998,3 @@
+
+TEST_F(FileSystemTest, PathFromFD) {
+  // Create a temp file.
----------------
Very nice! Can you also add a testcase for `openFileForRead` ?


http://reviews.llvm.org/D19842