[PATCH] D45233: [Driver] Update GCC libraries detection logic for Gentoo.

[Michał Górny via Phabricator via cfe-commits]

mgorny requested changes to this revision.
mgorny added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:2284
+        // Test the path based on the version in /etc/env.d/gcc/config-{tuple}.
+        GentooScanPaths.push_back(SysRootPrefix + "/usr/lib/gcc/" +
+                                  ActiveVersion.first.str() + "/" +
----------------
I'm not sure if there's a point in keeping this if you actually parse the config file. I can't think of a really valid case when GCC would be installed without LDPATH in the config file. Not saying it's wrong. You may want at least to push it after the configfile paths though.


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:2287
+                                  ActiveVersion.second.str());
+        // Scan the Config file to find installed GCC libaries path.
+        // A typical content of gcc config file:
----------------
Typo: 'libaries'


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:2296
+        if (ConfigFile) {
+         SmallVector<StringRef, 2> ConfigLines;
+          ConfigFile.get()->getBuffer().split(ConfigLines, "\n");
----------------
Misindent.


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:2300
+            ConfLine = ConfLine.trim();
+            if (ConfLine.consume_front("LDPATH=\"") &&
+                ConfLine.consume_back("\"")) {
----------------
This probably doesn't harm but I'd prefer if quotes weren't considered an obligatory part of the string. I'd rather grab it by `LDPATH=` and strip quotes afterwards if present on both sides. But I won't insist on this.


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:2303
+              std::pair<StringRef, StringRef> GentooLibs = ConfLine.split(':');
+              GentooScanPaths.push_back(GentooLibs.first.str());
+              if (!GentooLibs.second.empty()) {
----------------
Here you seem to assume that there would be at most 2 paths. That's a wrong assumption — there are triple-ABI targets (e.g. amd64 with x32 variant), and there is no reason prohibiting more LDPATHs. So please use the 'full' split, and iterate over all paths.


Repository:
  rC Clang

https://reviews.llvm.org/D45233