[PATCH] D35755: [Solaris] gcc toolchain handling revamp

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:1511
+  StringRef Suff64 = "/64";
+  // Solaris uses platform-specific suffixes instead of /64
+  if (TargetTriple.getOS() == llvm::Triple::Solaris) {
----------------
Add a period at the end of the comment.


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:1732
 
-    // Then look for distribution supplied gcc installations.
+    // Now prefix(es) that correspond to distribution-supplied gcc installations
     if (D.SysRoot.empty()) {
----------------
s/Now/Next, look for
Add a period at the end of the comment.


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:1734
     if (D.SysRoot.empty()) {
-      // Look for RHEL devtoolsets.
-      Prefixes.push_back("/opt/rh/devtoolset-6/root/usr");
-      Prefixes.push_back("/opt/rh/devtoolset-4/root/usr");
-      Prefixes.push_back("/opt/rh/devtoolset-3/root/usr");
-      Prefixes.push_back("/opt/rh/devtoolset-2/root/usr");
-      // And finally in /usr.
-      Prefixes.push_back("/usr");
+      // typically /usr
+      AddDefaultGCCPrefixes(TargetTriple, Prefixes, D.SysRoot);
----------------
s/typically/Typically
Add a period at the end of the comment.


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:1807
+void Generic_GCC::GCCInstallationDetector::AddDefaultGCCPrefixes(
+    const llvm::Triple &TargetTriple, SmallVectorImpl<std::string> &Prefixes,
+    StringRef SysRoot) {
----------------
I'd drop the `llvm::` here.


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:1813-1814
+    //   /usr/gcc/<major>.<minor>/lib/gcc/<triple>/<major>.<minor>.<patch>/
+    // so we need to find those /usr/gcc/*/lib/gcc libdirs
+    // and go with /usr/gcc/<version> as a prefix
+
----------------
This comment can probably be re-flowed, and needs a period at the end of it.


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:1837
+
+  // non-Solaris is much simpler - most systems just go with "/usr"
+  if (SysRoot.empty()) {
----------------
s/non/Non
Needs a period at the end of the comment.


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:1840
+    // Yet, still look for RHEL devtoolsets
+    // (should it be done Linux-only??)
+    Prefixes.push_back("/opt/rh/devtoolset-6/root/usr");
----------------
This is a good question to get an answer to before committing. :-) I don't know the answer myself, unfortunately.


================
Comment at: lib/Driver/ToolChains/Gnu.h:253
 
+    void AddDefaultGCCPrefixes(const llvm::Triple &TargetTriple,
+                               SmallVectorImpl<std::string> &Prefixes,
----------------
Might as well drop the `llvm::` since the namespace isn't used for `SmallVectorImpl`.


================
Comment at: lib/Driver/ToolChains/Gnu.h:309
+  // FIXME: This should be final, but CrossWindows toolchain does weird
+  // things that cant be easily generalized
   void AddClangCXXStdlibIncludeArgs(
----------------
s/but CrossWindows/but the CrossWindows
s/cant/can't
Add a period at the end of the comment.


================
Comment at: lib/Driver/ToolChains/Solaris.cpp:151
+  if (GCCInstallation.isValid()) {
+    // On Solaris gcc uses both architecture-specific path with triple in it
+    // as well as more generic lib path (+arch suffix)
----------------
s/both architecture/both an architecture


================
Comment at: lib/Driver/ToolChains/Solaris.cpp:152
+    // On Solaris gcc uses both architecture-specific path with triple in it
+    // as well as more generic lib path (+arch suffix)
+    addPathIfExists(D,
----------------
s/as more/as a more

Add a period at the end of the comment.


================
Comment at: lib/Driver/ToolChains/Solaris.cpp:160
 
-  addPathIfExists(D, getDriver().SysRoot + LibPath, Paths);
+  // if we are currently running Clang inside of the requested system root,
+  // add its parent library path to those searched.
----------------
s/if/If


================
Comment at: lib/Driver/ToolChains/Solaris.cpp:208
   if (GCCInstallation.isValid()) {
-    GCCVersion Version = GCCInstallation.getVersion();
-    addSystemInclude(DriverArgs, CC1Args,
-                     getDriver().SysRoot + "/usr/gcc/" +
-                     Version.MajorStr + "." +
-                     Version.MinorStr +
-                     "/include/c++/" + Version.Text);
-    addSystemInclude(DriverArgs, CC1Args,
-                     getDriver().SysRoot + "/usr/gcc/" + Version.MajorStr +
-                     "." + Version.MinorStr + "/include/c++/" +
-                     Version.Text + "/" +
-                     GCCInstallation.getTriple().str());
+    const auto &Callback = Multilibs.includeDirsCallback();
+    if (Callback) {
----------------
Shouldn't use `auto` here because the type is not spelled out in the initialization.


https://reviews.llvm.org/D35755