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