[PATCH] D142893: [NFC] Class for building MultilibSet

[Michael Platings via Phabricator via cfe-commits]

michaelplatings added inline comments.


================
Comment at: clang/include/clang/Driver/Multilib.h:25
 namespace driver {
 
 /// This corresponds to a single GCC Multilib, or a segment of one controlled
----------------
peter.smith wrote:
> It took a bit of reading to work out what the relationship between Multilib, MultilibVariant, MultilibSet and MultilibVariantBuilder are and how they are expected to be used.
> 
> IIUC MultilibVariant and MultilibVariantBuilder are used to construct Multilib and MultilibSet, which are expected to be immutable post construction.
> 
> Will be worth either a comment describing the relation ship or potentially write up the design in the clang docs and reference it from here. 
Yeah the naming was not intuitive, a hangover from one of the previous forms of this change. I've changed it so that `MultilibBuilder` creates `Multilib` and `MultilibSetBuilder` creates `MultilibSet`. That should make the comments less necessary, but I've added those too.


================
Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:32
 
-static Multilib makeMultilib(StringRef commonSuffix) {
-  return Multilib(commonSuffix, commonSuffix, commonSuffix);
+static MultilibBuilderVariant makeMultilib(StringRef commonSuffix) {
+  return MultilibBuilderVariant(commonSuffix, commonSuffix, commonSuffix);
----------------
peter.smith wrote:
> Worth making this a lambda in findRISCVMultilibs? Purely subjective opinion here as there could be an attempt to limit changes.
> there could be an attempt to limit changes.
Indeed. I'd rather leave this in place for that reason.


================
Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:44
   if (TargetTriple.isRISCV64()) {
-    Multilib Imac = makeMultilib("").flag("+march=rv64imac").flag("+mabi=lp64");
-    Multilib Imafdc = makeMultilib("/rv64imafdc/lp64d")
-                          .flag("+march=rv64imafdc")
-                          .flag("+mabi=lp64d");
+    auto Imac = makeMultilib("").flag("+march=rv64imac").flag("+mabi=lp64");
+    auto Imafdc = makeMultilib("/rv64imafdc/lp64d")
----------------
peter.smith wrote:
> Wondering if we've lost information here by going to auto. We're really making a MultilibVariant here. Perhaps worth renaming makeMultilib to makeMultilibVariant?
I've gone back to declaring the type explicitly and renamed the function to `makeMultilibBuilder` since `MultilibBuilder` is the return type now.


================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1049
 
-static Multilib makeMultilib(StringRef commonSuffix) {
-  return Multilib(commonSuffix, commonSuffix, commonSuffix);
+static MultilibBuilderVariant makeMultilib(StringRef commonSuffix) {
+  return MultilibBuilderVariant(commonSuffix, commonSuffix, commonSuffix);
----------------
peter.smith wrote:
> Similar comment to BareMetal, is it worth renaming to makeMultilibVariant
Renamed to `makeMultilibBuilder` since `MultilibBuilder` is the return type now.


================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1737
           });
 
   Multilib::flags_list Flags;
----------------
peter.smith wrote:
> Although I'm not too bothered myself, some people prefer to keep changes in whitespace to a separate NFC patch for the benefit of git annotate.
git-clang-format insists on changing it. A removed line won't show up in git annotate/blame (unless you're doing some extreme reverse-blaming activity, which mostly people don't) so I think it's best to let git-clang-format do its thing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142893/new/

https://reviews.llvm.org/D142893