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

[Eric Niebler via llvm-commits]

eric_niebler added a comment.

> What I'm suggesting here is that you abstract everything that guarantees returning the realpath (as in absolute, etc) into a separate function, lets call it `canonical_path`, even if that means that the only reliable option is calling the `realpath` function itself. Since on windows it's a different function, I see much value in having that abstracted out into a OS independent function.


This makes sense.

> Additionally, I believe some of the magic you are current doing could be used as a fast path to obtain the absolute path as well, right?


I haven't benchmarked that. I'm guessing that querying `/proc` is not going to be faster than `realpath`. `F_GETPATH` might be, but that's speculation at this point.

> This function can do any necessary magic but also call `canonical_path` whenever it needs OS independent `realpath` fallback. Does that make sense?


Actually, I'm unlikely to use any new `canonical_path` from `openFileForRead`. For a new `canonical_path` API, it'll actually need to get the answer right. That means fallbacks, retry logic, and long path handling. That's not appropriate for `openFileForRead`, whose job is primarily to open a file. It makes an opportunistic best-effort to get the real-case-name, and gives up quickly so as not to slow down the preprocessor.

So, I could implement a portable `canonical_path` API in this diff, but it won't be used from anywhere. That's why I've been resisting making this change. Just making sure that's understood. All that being said, I am happy to add it along with some tests if it means forward progress.


================
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)
+    return std::error_code();
----------------
bruno wrote:
> space between "if" and "("
Yup, thx.


http://reviews.llvm.org/D19842