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

In http://reviews.llvm.org/D19842#437302, @eric_niebler wrote:

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


I agree, definitely not the place. After re-reading your patch I also agree that introducing  `canonical_path` doesn't help much. However, it's very convenient to have `bool getPathFromOpenFD(StringRef Filename, SmallVectorImpl<char> *RealPath)` or something along these lines. The reason is that we can then re-use the logic if we find appropriate when implementing `canonical_path`. How does that sound?

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


Don't bother :-)

We need tests for this, can you add some? Ideally unittests (somewhere in unittests/Support).


http://reviews.llvm.org/D19842