[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.

[David Majnemer via llvm-commits]

majnemer added a subscriber: majnemer.

================
Comment at: lib/Support/Unix/Path.inc:522
@@ +521,3 @@
+  // Attempt to get the real name of the file, if the user asked
+  if(RealPath) {
+    RealPath->clear();
----------------
I'd go for an early return to reduce indentation.

================
Comment at: lib/Support/Unix/Path.inc:524-547
@@ +523,26 @@
+    RealPath->clear();
+  #if 0 //__APPLE__
+    // On MacOS, fgetattrlist is the most reliable way to get a properly-cased
+    // path.
+    // BUGBUG commented out since fgetattrlist/ATTR_CMN_NAME seems to only
+    // fetch the file *name* and not the path.
+    struct attrlist attrList;
+    memset(&attrList, 0, sizeof(attrList));
+    attrList.bitmapcount = ATTR_BIT_MAP_COUNT;
+    attrList.commonattr = ATTR_CMN_NAME;
+    struct NameAttr {
+      u_int32_t totalByteCount;
+      struct attrreference attrRef;
+      char Name[NAME_MAX * 3 + 1]; // Large enough for NAME_MAX UTF8 characters
+    };
+    union {
+      NameAttr nameAttr;
+      char Buffer[sizeof(nameAttr)];
+    };
+    if (fgetattrlist(ResultFD, &attrList, Buffer, sizeof(Buffer), 0) != -1) {
+      assert(nameAttr.attrRef.attr_dataoffset == sizeof(struct attrreference));
+      if (0 < nameAttr.attrRef.attr_length)
+        RealPath->append(nameAttr.Name,
+                         nameAttr.Name + nameAttr.attrRef.attr_length - 1);
+    }
+  #elif defined(F_GETPATH)
----------------
Please remember to remove code which is disabled if you check it in.

================
Comment at: lib/Support/Unix/Path.inc:554-556
@@ +553,5 @@
+      RealPath->append(Buffer, Buffer + strlen(Buffer));
+  #else
+    // If we have a /proc filesystem mounted, we ca quickly establish the
+    // real name of the file with readlink
+    static const bool HasProc = (0 == ::access("/proc/self/fd", R_OK));
----------------
This should be `#ifdef __linux__`

================
Comment at: lib/Support/Unix/Path.inc:557
@@ +556,3 @@
+    // real name of the file with readlink
+    static const bool HasProc = (0 == ::access("/proc/self/fd", R_OK));
+    char Buffer[PATH_MAX];
----------------
I wouldn't bother with this, it has a non-trivial amount of overhead on non-X86 platforms.  I'd just fall-back to `::realpath` if `::readlink` failed.

In fact, why not fall back to `::realpath` regardless of platform?

================
Comment at: lib/Support/Unix/Path.inc:560-561
@@ +559,4 @@
+    if (HasProc) {
+      char ProcPath[64];
+      int N = snprintf(ProcPath, sizeof(ProcPath), "/proc/self/fd/%d", ResultFD);
+      assert(N > 0 && N < sizeof(ProcPath));
----------------
I think it'd be nicer to build a twine like so:
  SmallString<32> ProcPathStorage;
  StringRef ProcPath = Twine("/proc/self/fd/").concat(ResultFD).toNullTerminatedStringRef(ProcPathStorage);

================
Comment at: lib/Support/Unix/Path.inc:563-565
@@ +562,5 @@
+      assert(N > 0 && N < sizeof(ProcPath));
+      ssize_t CharCount = ::readlink(ProcPath, Buffer, sizeof(Buffer));
+      if (CharCount > 0)
+        RealPath->append(Buffer, Buffer + CharCount);
+    } else {
----------------
What's the contract on failure?  Should `RealPath` be cleared?

================
Comment at: lib/Support/Windows/Path.inc:701
@@ +700,3 @@
+    DWORD CountChars =
+      ::GetFinalPathNameByHandleW(H, RealPathUTF16, MAX_PATH, 0);
+    if (CountChars > 0 && CountChars < MAX_PATH) {
----------------
Could we use `FILE_NAME_NORMALIZED` instead of `0`?

================
Comment at: lib/Support/Windows/Path.inc:702
@@ +701,3 @@
+      ::GetFinalPathNameByHandleW(H, RealPathUTF16, MAX_PATH, 0);
+    if (CountChars > 0 && CountChars < MAX_PATH) {
+      // Convert the result from UTF-16 to UTF-8.
----------------
Seeing as how we support extended-length paths, it'd be nice to continue supporting them here.  Perhaps we could retry similarly to what we do in `Process::GetEnv` ?

================
Comment at: lib/Support/Windows/Path.inc:704
@@ +703,3 @@
+      // Convert the result from UTF-16 to UTF-8.
+      SmallVector<char, MAX_PATH> RealPathUTF8;
+      if (!UTF16ToUTF8(RealPathUTF16, CountChars, RealPathUTF8))
----------------
I'd use `SmallString<MAX_PATH> RealPathUTF8;`


http://reviews.llvm.org/D19842