[PATCH] D37954: Try to shorten system header paths when using -MD depfiles

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In https://reviews.llvm.org/D37954#884029, @Lekensteyn wrote:

> Any objection for merging these patches? I rebased today (no changes needed) and it still passes clang-tests.


This is not the way that we do code review in LLVM and Clang. If you don't get a response to a patch, you're expected to keep pinging it (typically once a week) until you do get an answer. This past week in particular has been especially bad as we had the mailing deadline for the C++ committee meeting on Monday and the LLVM developer meeting yesterday and today.

I reverted this in r316195, because it breaks users using `-fno-canonical-prefixes` to request that we do not do this kind of path canonicalization.



================
Comment at: cfe/trunk/lib/Driver/ToolChains/Clang.cpp:967-968
 
+  if (Arg *A = Args.getLastArg(options::OPT_fno_canonical_system_headers,
+                               options::OPT_fcanonical_system_headers)) {
+    if (A->getOption().matches(options::OPT_fno_canonical_system_headers)) {
----------------
Use `hasFlag` instead.

Also, defaulting this on will break existing users who use `-fno-canonical-prefixes` to turn off the (wrong in general) assumption that using `realpath` is meaningful. Please default to the value of that flag instead. I would guess (but haven't checked) that flag controls this behavior in GCC.


================
Comment at: cfe/trunk/lib/Frontend/DependencyFile.cpp:296
+  if (CanonicalSystemHeaders && isSystem(FileType)) {
+    StringRef RealPath = FE->tryGetRealPathName();
+    if (!RealPath.empty() && RealPath.size() < Filename.size()) {
----------------
This is not an appropriate use of `tryGetRealPathName`. Its purpose is to get the way the file name was stored in disk (particularly, preserving the original case on a case-sensitive file system), not to resolve symlinks.

`FileManager::getCanonicalName` would be the right thing to call.


Repository:
  rL LLVM

https://reviews.llvm.org/D37954