[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
llvm-commits at lists.llvm.org
Mon May 23 16:14:46 PDT 2016
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
+ return std::error_code();
> space between "if" and "("
More information about the llvm-commits