michaelplatings added inline comments.
================
Comment at: clang/include/clang/Driver/Multilib.h:25
namespace driver {
/// This corresponds to a single GCC Multilib, or a segment of one controlled
----------------
peter.smith wrote:
> 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.
Yeah the naming was not intuitive, a hangover from one of the previous forms of
this change. I've changed it so that `MultilibBuilder` creates `Multilib` and
`MultilibSetBuilder` creates `MultilibSet`. That should make the comments less
necessary, but I've added those too.
================
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);
----------------
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.
================
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")
----------------
peter.smith wrote:
> Wondering if we've lost information here by going to auto. We're really
> making a MultilibVariant here. Perhaps worth renaming makeMultilib to
> makeMultilibVariant?
I've gone back to declaring the type explicitly and renamed the function to
`makeMultilibBuilder` since `MultilibBuilder` is the return type now.
================
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);
----------------
peter.smith wrote:
> Similar comment to BareMetal, is it worth renaming to makeMultilibVariant
Renamed to `makeMultilibBuilder` since `MultilibBuilder` is the return type now.
================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1737
});
Multilib::flags_list Flags;
----------------
peter.smith wrote:
> 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.
git-clang-format insists on changing it. A removed line won't show up in git
annotate/blame (unless you're doing some extreme reverse-blaming activity,
which mostly people don't) so I think it's best to let git-clang-format do its
thing.
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
