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

[Michael Platings via Phabricator via cfe-commits]

michaelplatings marked 4 inline comments as done.
michaelplatings 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;
----------------
phosek wrote:
> 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.
Immutable was overstating it. I won't rule out that there may be a good reason to mutate it in future, but there's no need to actively support mutation now. I removed "immutable" from the comment.


================
Comment at: clang/include/clang/Driver/Multilib.h:85
 public:
   using multilib_list = std::vector<Multilib>;
   using const_iterator = multilib_list::const_iterator;
----------------
phosek wrote:
> 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.
Immutable was overstating it, and wasn't intended to apply to this class - a later change adds a parse() method which mutates in-place. I'm also sceptical of TODOs ever getting done - there are over 3,000 of them in `llvm/lib` alone :(


================
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);
 
----------------
phosek wrote:
> 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`.
Immutable was overstating it, and wasn't intended to apply to this class - a later change adds a `parse()` method which mutates in-place. I did start off by removing push_back() but found it made the class less convenient to use.


================
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);
----------------
phosek wrote:
> 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.
I've gone ahead and added the constructor in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142893