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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
