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

[Petr Hosek via Phabricator via cfe-commits]

phosek added inline comments.


================
Comment at: clang/include/clang/Driver/Multilib.h:35-39
   std::string GCCSuffix;
   std::string OSSuffix;
   std::string IncludeSuffix;
   flags_list Flags;
   int Priority;
----------------
Since this class is intended to be immutable, we should also consider marking all fields as `const`. This can be done in a future refactoring, but it might be worth leaving a `TODO` here.


================
Comment at: clang/include/clang/Driver/Multilib.h:85
 public:
   using multilib_list = std::vector<Multilib>;
   using const_iterator = multilib_list::const_iterator;
----------------
Since this class is intended to be immutable, and the number of `Multilib`s is known at construction time, we could use `llvm::TrailingObjects` instead of `std::vector` which would be more efficient. This can be done in a future refactoring, but it might be worth leaving a `TODO` here.


================
Comment at: clang/include/clang/Driver/Multilib.h:105-106
 
-  /// Filter out those Multilibs whose gccSuffix matches the given expression
-  MultilibSet &FilterOut(const char *Regex);
-
   /// Add a completed Multilib to the set
   void push_back(const Multilib &M);
 
----------------
I don't think we should expose this method to maintain the invariant that `MultilibSet` is immutable post construction, instead this method should be provided by `MultilibSetBuilder`.


================
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);
----------------
michaelplatings wrote:
> 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.
I'd suggest adding a single argument constructor to `MultilibBuilder` since this pattern is replicated across different files and so it makes sense to generalize but I agree that we can do that in a follow up change as a cleanup/refactor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142893