nickdesaulniers added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:360-361
// CSKY toolchains use different names for sysroot folder.
if (!GCCInstallation.isValid())
return std::string();
// GCCInstallation.getInstallPath() =
----------------
hoist
================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:365-366
// Path = $GCCToolchainPath/csky-linux-gnuabiv2/libc
std::string Path = (GCCInstallation.getInstallPath() + "/../../../../" +
GCCInstallation.getTriple().str() + "/libc")
.str();
----------------
duplication with below...
================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:373-374
- if (!GCCInstallation.isValid() || !getTriple().isMIPS())
+ if (!GCCInstallation.isValid())
return std::string();
----------------
Cool, now this can be hoisted above `if (getTriple().isCSKY()) {` and not
checked again within that condition.
================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:376
- // Standalone MIPS toolchains use different names for sysroot folder
- // and put it into different places. Here we try to check some known
- // variants.
-
+ const Distro Distro(getDriver().getVFS(), getTriple());
const StringRef InstallDir = GCCInstallation.getInstallPath();
----------------
I see that `getVFS()` is called already without going through `getDriver()` in
this method. Can we do so as well here?
================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:377-389
const StringRef InstallDir = GCCInstallation.getInstallPath();
const StringRef TripleStr = GCCInstallation.getTriple().str();
const Multilib &Multilib = GCCInstallation.getMultilib();
+ std::string BasePath = (InstallDir + "/../../../../" + TripleStr).str();
- std::string Path =
- (InstallDir + "/../../../../" + TripleStr + "/libc" +
Multilib.osSuffix())
- .str();
-
- if (getVFS().exists(Path))
- return Path;
+ if (Distro.IsArchLinux() && getVFS().exists(BasePath))
+ return BasePath;
----------------
...seems like some of this is duplicated with CSKY above. Can you please
refactor the MIPS and CSKY checks to reuse more code?
I doubt arch-linux has a CSKY port; I suspect you might be able to check for
arch-linux first, then do the CSKY and MIPS special casing after.
All this code is doing is figuring out a Path, then if it exists then return
it. Feel like is should be:
```
if android:
path = ...
elif arch:
path = ...
elif csky:
path = ...
elif mips:
path = ...
else:
return ""
if path.exists():
return path
return ""
```
================
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())
----------------
10ne1 wrote:
> nickdesaulniers wrote:
> > 10ne1 wrote:
> > > nickdesaulniers wrote:
> > > > 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.
> > > In my testing on ArchLinux, `GCCInstallation.isValid()` is always `true`.
> > >
> > > The problem is that if test condition doesn't just test for a valid GCC
> > > install, it also tests `getTriple().isMIPS()` which is always false on
> > > Arch Linux which does not support mips, so the sysroot will always be an
> > > empty string.
> > >
> > > If you wish I can create a separate commit / review to untangle `if
> > > (!GCCInstallation.isValid() || !getTriple().isMIPS())` and try to reduce
> > > the code duplication, because really having a valid GCC install is
> > > independent from running on mips. What do you think?
> > That doesn't make sense.
> >
> > If `GCCInstallation.isValid()` is always `true` then we should not be
> > returning the empty string.
> It does makes sense if you read the condition carefully:
>
> ```
> if (!GCCInstallation.isValid() || !getTriple().isMIPS())
> ```
>
> results in:
>
> ```
> if ( ! true || ! false )
> ```
>
> which means an empty string is always returned because Arch does not support
> mips even though a valid GCC install is present.
>
> I think I'll just go ahead and clean up this code and update the diff to drop
> the triple normalization.
>
>
Ah, ok. Yeah your refactoring is making this clearer. I think we can refactor
this method a bit more (in addition to fixing this Arch-Linux specific issue).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134454/new/
https://reviews.llvm.org/D134454
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
