[PATCH] D134454: [Driver][Distro] Fix ArchLinux triplet and sysroot detection

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Is it worth contacting the package maintainer for LLVM+clang for Arch-Linux in regards to this patch?



================
Comment at: clang/lib/Driver/Distro.cpp:210
+  // Sometimes the OS can't be detected from the triplet due to ambiguity, for
+  // eg. ArchLinux uses aarch64-linux-gnu which results in Uknonwn OS & distro,
+  // so normalize the triplet which results in aarch64-unknown-linux-gnu, such
----------------
typo


================
Comment at: clang/lib/Driver/Distro.cpp:213
+  // that the Linux OS and distro are properly detected in this cases.
+  llvm::Triple NormTargetOrHost = llvm::Triple(Twine(TargetOrHost.normalize()));
+
----------------
Twine has an intentionally non-explicit constructor that accepts a StringRef, which also has an intentionally non-explicit constructor that accepts a std::string, which is what Triple::normalize() returns.

Let's be consistent in the explicit construction of these temporary objects by removing the explicit call to the Twine constructor.

Also, why is it necessary to normalize the Triple? Can you give an example of the "bad" input and how this "fixes" it?


================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:377-394
+  if (Distro.IsArchLinux()) {
+    std::string Path = (InstallDir + "/../../../../"  + TripleStr).str();
+    if (getVFS().exists(Path))
+      return Path;
+  }
+
   if (!GCCInstallation.isValid() || !getTriple().isMIPS())
----------------
Do we need the Arch-linux test before the call to `GCCInstallation.isValid()`? Otherwise it seems like this duplicates a fair amount of code computing the `Path`.  The initial parts of the Path seem to match; there's only more to it if the Distro is not arch-linux.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134454/new/

https://reviews.llvm.org/D134454