[PATCH] D45604: Support for multiarch runtimes layout

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added inline comments.


================
Comment at: clang/lib/Driver/ToolChain.cpp:90
       CachedRTTIMode(CalculateRTTIMode(Args, Triple, CachedRTTIArg)) {
+#ifdef CLANG_ENABLE_PER_TARGET_RUNTIME_DIR
+  SmallString<128> P(D.ResourceDir);
----------------
Maybe use `cmakedefine01` and change this to a simple `if`?


================
Comment at: clang/lib/Driver/ToolChain.cpp:392
                                     Arch + Env + Suffix);
   return Path.str();
+#endif
----------------
These last two lines can probably get hoisted out of the `if` and make it easier to notice the difference between the two paths. Same comment as above about using a "normal if" instead of `#if`.


================
Comment at: clang/lib/Driver/ToolChains/Fuchsia.cpp:152
+  Triple.setTriple(Triple.getArchName() + "-" + Triple.getOSName());
+  return Triple.str();
 }
----------------
Why "compute triple + set triple + return Triple.str()" instead of a plain "compute triple + return arch + '-' + os"?


================
Comment at: clang/lib/Driver/ToolChains/Fuchsia.h:69
+      const llvm::opt::ArgList &Args,
+      types::ID InputType = types::TY_INVALID) const override;
 
----------------
Why the extra default argument? The patch doesn't seem to show any call to this function that requires a single arg.


Repository:
  rL LLVM

https://reviews.llvm.org/D45604