peter.smith added a comment.
Looks good so far. Some stylistic suggestions.
================
Comment at: clang/include/clang/Driver/Multilib.h:25
namespace driver {
/// This corresponds to a single GCC Multilib, or a segment of one controlled
----------------
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.
================
Comment at: clang/include/clang/Driver/Multilib.h:40
public:
Multilib(StringRef GCCSuffix = {}, StringRef OSSuffix = {},
+ StringRef IncludeSuffix = {}, int Priority = 0,
----------------
Could be worth documenting the requirement for the strings to start with a `/`
or be empty as the constructor implementation in a different file will assert
if they aren't?
================
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);
----------------
Worth making this a lambda in findRISCVMultilibs? Purely subjective opinion
here as there could be an attempt to limit changes.
================
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")
----------------
Wondering if we've lost information here by going to auto. We're really making
a MultilibVariant here. Perhaps worth renaming makeMultilib to
makeMultilibVariant?
================
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);
----------------
Similar comment to BareMetal, is it worth renaming to makeMultilibVariant
================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1737
});
Multilib::flags_list Flags;
----------------
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.
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
