nickdesaulniers added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:363
const StringRef TripleStr = GCCInstallation.getTriple().str();
- const Multilib &Multilib = GCCInstallation.getMultilib();
+ std::string Path = (InstallDir + "/../../../../" + TripleStr).str();
----------------
With all of the string concatenation going on, I wonder if `Path` should be a
`Twine`? `llvm::vfs::Filesystem::exists` accepts a `const Twine&`. That
avoids multiple reallocations and copies, and does one lazily.
(Every time I've tried to use `Twine`, I wind up with either `-Wdangling-gsl`
or segfaults though! Still, please give it a shot.)
================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:369-375
+ if (getTriple().isCSKY())
+ Path = Path + "/libc";
- if (getVFS().exists(Path))
- return Path;
+ // Standalone MIPS toolchains use different names for sysroot folder
+ // and put it into different places. Here we try to check some known
+ // variants.
+ if (getTriple().isMIPS()) {
----------------
CSKY and MIPS should be mutually exclusive. Please make this second `if` an
`else if`.
```
if csky:
...
elif mips:
...
```
================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:370
+ if (getTriple().isCSKY())
+ Path = Path + "/libc";
----------------
+=
================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:373
+ // Standalone MIPS toolchains use different names for sysroot folder
+ // and put it into different places. Here we try to check some known
+ // variants.
----------------
Specifically there's 2 known variants for mips, it looks like. Please update
this comment.
================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:376
+ if (getTriple().isMIPS()) {
+ const Multilib &Multilib = GCCInstallation.getMultilib();
+ Path = Path + "/libc" + Multilib.osSuffix();
----------------
It might be worthwhile to have the variable be
```
std::string OSSuffix = GCCInstallation.getMultilib().osSuffix();
```
rather than the `Multilib` object.
================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:377
+ const Multilib &Multilib = GCCInstallation.getMultilib();
+ Path = Path + "/libc" + Multilib.osSuffix();
+ if (getVFS().exists(Path))
----------------
+=
================
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;
----------------
10ne1 wrote:
> nickdesaulniers wrote:
> > ...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 ""
> > ```
> Thanks for this suggestion. Indeed after doing all the simplifications I have
> some great news: we do not need to test if Distro == ArchLinux because the
> sysroot path it uses is the implicit base path one common to all
> architectures!
>
> So just doing the following is enough to also fix ArchLinux:
>
> ```
> std::string Path = (InstallDir + "/../../../../" + TripleStr).str();
>
> if (getTriple().isCSKY())
> Path = Path + "/libc";
>
> if (getTriple().isMIPS()) {
> ...
> }
>
> if (getVFS().exists(Path))
> return Path;
>
> return std::string();
> ```
Yes, this is much nicer in that we don't have to special case arch-linux.
Sometimes refactorings beget more refactorings; it's nice when that happens.
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
