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

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Looks good so far. Some stylistic suggestions.



================
Comment at: clang/include/clang/Driver/Multilib.h:25
 namespace driver {
 
 /// This corresponds to a single GCC Multilib, or a segment of one controlled
----------------
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. 


================
Comment at: clang/include/clang/Driver/Multilib.h:40
 public:
   Multilib(StringRef GCCSuffix = {}, StringRef OSSuffix = {},
+           StringRef IncludeSuffix = {}, int Priority = 0,
----------------
Could be worth documenting the requirement for the strings to start with a `/` or be empty as the constructor implementation in a different file will assert if they aren't?


================
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);
----------------
Worth making this a lambda in findRISCVMultilibs? Purely subjective opinion here as there could be an attempt to limit changes.


================
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")
----------------
Wondering if we've lost information here by going to auto. We're really making a MultilibVariant here. Perhaps worth renaming makeMultilib to makeMultilibVariant?


================
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);
----------------
Similar comment to BareMetal, is it worth renaming to makeMultilibVariant


================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1737
           });
 
   Multilib::flags_list Flags;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142893