Page MenuHomePhabricator

Do not resolve directory junctions for `-fdiagnostics-absolute-paths` on Windows.
ClosedPublic

Authored by ikudrin on Mar 15 2019, 8:01 AM.

Details

Summary

This patch partially restores the behavior changed in D46942/rC332590

If the source file path contains directory junctions, and we resolve them when printing
diagnostic messages, these paths look independent for an IDE. For example, both Visual
Studio and Visual Studio Code open separate editors for such paths, which is not only
inconvenient but might even result in loss of the changes made in one of them.

Diff Detail

Repository
rC Clang

Event Timeline

ikudrin created this revision.Mar 15 2019, 8:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2019, 8:01 AM
hans added inline comments.Mar 18 2019, 2:49 AM
test/Frontend/absolute-paths-windows.test
5

This requires (a recent version of) Win 10 or later to run without running as Admin, right?

ikudrin marked an inline comment as done.Mar 18 2019, 4:49 AM
ikudrin added inline comments.
test/Frontend/absolute-paths-windows.test
5

As far as I know, creating a directory junction, in contrast to a symbolic link, does not require escalated privileges. But, honestly, I do not have any old system to validate that.

asl added a subscriber: asl.Mar 18 2019, 11:51 AM
ikudrin marked an inline comment as done.Mar 19 2019, 12:04 AM
ikudrin added inline comments.
test/Frontend/absolute-paths-windows.test
5

We checked that on Windows 7. It does not require rights elevation, too.

Ping.

Please note, that, in contrary to POSIX OSes, on Windows a path like dir1\<junction point>\.. refers to dir1. That's why we do not need to ask the system for the fully expanded path and can do all calculations internally.

hans added a comment.Apr 10 2019, 5:57 AM

I don't feel like I have enough background on what you're trying to do to review this. It sounds related to clangd issues maybe?

"If the source file path contains directory junctions, and we resolve them when printing diagnostic messages"

I didn't think Clang tried to resolve symlinks or otherwise canonicalize paths when refering to files in diagnostics in the first place?

ikudrin updated this revision to Diff 200252.May 20 2019, 4:35 AM
  • Made the patch affect only -fdiagnostics-absolute-paths option.
hans added inline comments.Wed, May 22, 4:20 AM
lib/Frontend/TextDiagnostic.cpp
768

I think this requires a comment explaining why Windows is handled differently.

ikudrin updated this revision to Diff 200749.Wed, May 22, 8:11 AM
ikudrin edited the summary of this revision. (Show Details)

Added a comment explaining the differences in the implementations. I would really appreciate any corrections.

hans accepted this revision.Thu, May 23, 12:28 AM

Thanks! This looks good to me.

I'm still a bit nervous about "mklink" in the test, but let's land this and see if the bots can handle it.

This revision is now accepted and ready to land.Thu, May 23, 12:28 AM
This revision was automatically updated to reflect the committed changes.