[PATCH] D146757: [Driver] Enable defining multilib print options independently of flags
michaelplatings updated this revision to Diff 518997.
michaelplatings added a comment.
Chagne code to allow defining multilib flags verbatim
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146757/new/
https://reviews.llvm.org/D146757
Files:
clang/include/clang/Driver/Multilib.h
clang/lib/Driver/Multilib.cpp
clang/unittests/Driver/MultilibBuilderTest.cpp
clang/unittests/Driver/MultilibTest.cpp
Index: clang/unittests/Driver/MultilibTest.cpp
===
--- clang/unittests/Driver/MultilibTest.cpp
+++ clang/unittests/Driver/MultilibTest.cpp
@@ -187,3 +187,18 @@
EXPECT_EQ("/a", Selection[0].gccSuffix());
EXPECT_EQ("/b", Selection[1].gccSuffix());
}
+
+TEST(MultilibBuilder, PrintOptions) {
+ ASSERT_EQ(Multilib::flags_list(), Multilib().getPrintOptions());
+ ASSERT_EQ(Multilib::flags_list({"-x"}),
+Multilib({}, {}, {}, {"+x"}).getPrintOptions());
+ ASSERT_EQ(Multilib::flags_list({"-x"}),
+Multilib({}, {}, {}, {"+x"}, Multilib::PrintOptionsType::PlusFlags)
+.getPrintOptions());
+ ASSERT_EQ(
+ Multilib::flags_list({"-x", "-a", "-c"}),
+ Multilib({}, {}, {}, {"+x", "-y", "+a", "-b", "+c"}).getPrintOptions());
+ ASSERT_EQ(Multilib::flags_list({"-y"}),
+Multilib({}, {}, {}, {"-y"}, Multilib::PrintOptionsType::Verbatim)
+.getPrintOptions());
+}
Index: clang/unittests/Driver/MultilibBuilderTest.cpp
===
--- clang/unittests/Driver/MultilibBuilderTest.cpp
+++ clang/unittests/Driver/MultilibBuilderTest.cpp
@@ -207,3 +207,14 @@
<< "Selection picked " << Selection << " which was not expected ";
}
}
+
+TEST(MultilibBuilderTest, PrintOptions) {
+ Multilib M = MultilibBuilder()
+ .flag("+x")
+ .flag("-y")
+ .flag("+a")
+ .flag("-b")
+ .flag("+c")
+ .makeMultilib();
+ ASSERT_EQ(Multilib::flags_list({"-x", "-a", "-c"}), M.getPrintOptions());
+}
Index: clang/lib/Driver/Multilib.cpp
===
--- clang/lib/Driver/Multilib.cpp
+++ clang/lib/Driver/Multilib.cpp
@@ -17,18 +17,17 @@
#include "llvm/Support/Path.h"
#include "llvm/Support/Regex.h"
#include "llvm/Support/raw_ostream.h"
-#include
#include
-#include
using namespace clang;
using namespace driver;
using namespace llvm::sys;
Multilib::Multilib(StringRef GCCSuffix, StringRef OSSuffix,
- StringRef IncludeSuffix, const flags_list )
+ StringRef IncludeSuffix, const flags_list ,
+ PrintOptionsType PrintOptions)
: GCCSuffix(GCCSuffix), OSSuffix(OSSuffix), IncludeSuffix(IncludeSuffix),
- Flags(Flags) {
+ Flags(Flags), PrintOptions(PrintOptions) {
assert(GCCSuffix.empty() ||
(StringRef(GCCSuffix).front() == '/' && GCCSuffix.size() > 1));
assert(OSSuffix.empty() ||
@@ -37,6 +36,28 @@
(StringRef(IncludeSuffix).front() == '/' && IncludeSuffix.size() > 1));
}
+static Multilib::flags_list
+getPrintOptionsFromPlusFlags(const Multilib::flags_list ) {
+ // Derive print options from flags beginning '+', replacing the first
+ // character with '-'.
+ Multilib::flags_list PrintOptions;
+ for (StringRef Flag : Flags) {
+if (Flag.front() == '+')
+ PrintOptions.push_back(("-" + Flag.substr(1)).str());
+ }
+ return PrintOptions;
+}
+
+Multilib::flags_list Multilib::getPrintOptions() const {
+ switch (PrintOptions) {
+ case PrintOptionsType::Verbatim:
+return Flags;
+ case PrintOptionsType::PlusFlags:
+return getPrintOptionsFromPlusFlags(Flags);
+ }
+ llvm_unreachable("Unknown Multilib::PrintOptionsType");
+}
+
LLVM_DUMP_METHOD void Multilib::dump() const {
print(llvm::errs());
}
@@ -44,14 +65,11 @@
void Multilib::print(raw_ostream ) const {
if (GCCSuffix.empty())
OS << ".";
- else {
+ else
OS << StringRef(GCCSuffix).drop_front();
- }
OS << ";";
- for (StringRef Flag : Flags) {
-if (Flag.front() == '+')
- OS << "@" << Flag.substr(1);
- }
+ for (StringRef Option : getPrintOptions())
+OS << "@" << Option.substr(1);
}
bool Multilib::operator==(const Multilib ) const {
Index: clang/include/clang/Driver/Multilib.h
===
--- clang/include/clang/Driver/Multilib.h
+++ clang/include/clang/Driver/Multilib.h
@@ -31,19 +31,32 @@
public:
using flags_list = std::vector;
+ /// When -print-multi-lib is used, this defines how the printed options must
+ /// be calculated.
+ enum class PrintOptionsType {
+/// Print Flags verbatim
+Verbatim,
+/// Print flags in the Flags list that begin with '+', but with the initial
+/// '+' replaced with '-'. e.g. "+fno-rtti" becomes "-fno-rtti"
+PlusFlags,
+ };
+
[PATCH] D146757: [Driver] Enable defining multilib print options independently of flags
michaelplatings added inline comments.
Comment at: clang/include/clang/Driver/Multilib.h:58
+ StringRef IncludeSuffix = {}, const flags_list =
flags_list(),
+ PrintOptionsType PrintOptions = PrintOptionsType::PlusFlags,
+ const flags_list = flags_list());
peter.smith wrote:
> How would someone using `makeMultilib()` from `MultilibBuilder` use these
> parameters? If they can't do they need to? If so we may need something like a
> makeMultilib overload to supply the extra parameters.
MultilibBuilder is constructed around the concept of using '+' and '-' to
indicate which command line options are compatible and incompatible, so in that
case you'd only want to use PlusFlags.
Comment at: clang/lib/Driver/Multilib.cpp:44
+ assert(PrintOptions == PrintOptionsType::List || PrintOptionsList.empty());
+}
+
peter.smith wrote:
> If there are any restrictions on the strings in PrintOptionsList could be
> worth assertions. Please ignore if there are no restrictions.
There are restrictions - PrintOptionsList should only contain valid Clang
options. However if you violate this then the only consequence is that the
-print-multi-lib output will be equally invalid. Since there would be a lot of
complexity involved in enforcing the restriction I think it's best to leave it
unenforced.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146757/new/
https://reviews.llvm.org/D146757
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146757: [Driver] Enable defining multilib print options independently of flags
michaelplatings updated this revision to Diff 511073.
michaelplatings marked 3 inline comments as done.
michaelplatings added a comment.
Expand constructor comment as suggested by @peter.smith
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146757/new/
https://reviews.llvm.org/D146757
Files:
clang/include/clang/Driver/Multilib.h
clang/lib/Driver/Multilib.cpp
clang/unittests/Driver/MultilibBuilderTest.cpp
clang/unittests/Driver/MultilibTest.cpp
Index: clang/unittests/Driver/MultilibTest.cpp
===
--- clang/unittests/Driver/MultilibTest.cpp
+++ clang/unittests/Driver/MultilibTest.cpp
@@ -187,3 +187,19 @@
EXPECT_EQ("/a", Selection[0].gccSuffix());
EXPECT_EQ("/b", Selection[1].gccSuffix());
}
+
+TEST(MultilibBuilder, PrintOptions) {
+ ASSERT_EQ(Multilib::flags_list(), Multilib().getPrintOptions());
+ ASSERT_EQ(Multilib::flags_list({"-x"}),
+Multilib({}, {}, {}, {"+x"}).getPrintOptions());
+ ASSERT_EQ(Multilib::flags_list({"-x"}),
+Multilib({}, {}, {}, {"+x"}, Multilib::PrintOptionsType::PlusFlags)
+.getPrintOptions());
+ ASSERT_EQ(
+ Multilib::flags_list({"-x", "-a", "-c"}),
+ Multilib({}, {}, {}, {"+x", "-y", "+a", "-b", "+c"}).getPrintOptions());
+ ASSERT_EQ(
+ Multilib::flags_list({"-y"}),
+ Multilib({}, {}, {}, {"+x"}, Multilib::PrintOptionsType::List, {"-y"})
+ .getPrintOptions());
+}
Index: clang/unittests/Driver/MultilibBuilderTest.cpp
===
--- clang/unittests/Driver/MultilibBuilderTest.cpp
+++ clang/unittests/Driver/MultilibBuilderTest.cpp
@@ -207,3 +207,14 @@
<< "Selection picked " << Selection << " which was not expected ";
}
}
+
+TEST(MultilibBuilderTest, PrintOptions) {
+ Multilib M = MultilibBuilder()
+ .flag("+x")
+ .flag("-y")
+ .flag("+a")
+ .flag("-b")
+ .flag("+c")
+ .makeMultilib();
+ ASSERT_EQ(Multilib::flags_list({"-x", "-a", "-c"}), M.getPrintOptions());
+}
Index: clang/lib/Driver/Multilib.cpp
===
--- clang/lib/Driver/Multilib.cpp
+++ clang/lib/Driver/Multilib.cpp
@@ -26,15 +26,43 @@
using namespace llvm::sys;
Multilib::Multilib(StringRef GCCSuffix, StringRef OSSuffix,
- StringRef IncludeSuffix, const flags_list )
+ StringRef IncludeSuffix, const flags_list ,
+ PrintOptionsType PrintOptions,
+ const flags_list )
: GCCSuffix(GCCSuffix), OSSuffix(OSSuffix), IncludeSuffix(IncludeSuffix),
- Flags(Flags) {
+ Flags(Flags), PrintOptionsList(PrintOptionsList),
+ PrintOptions(PrintOptions) {
assert(GCCSuffix.empty() ||
(StringRef(GCCSuffix).front() == '/' && GCCSuffix.size() > 1));
assert(OSSuffix.empty() ||
(StringRef(OSSuffix).front() == '/' && OSSuffix.size() > 1));
assert(IncludeSuffix.empty() ||
(StringRef(IncludeSuffix).front() == '/' && IncludeSuffix.size() > 1));
+ // If PrintOptions is something other than List then PrintOptionsList must be
+ // empty.
+ assert(PrintOptions == PrintOptionsType::List || PrintOptionsList.empty());
+}
+
+static Multilib::flags_list
+getPrintOptionsFromPlusFlags(const Multilib::flags_list ) {
+ // Derive print options from flags beginning '+', replacing the first
+ // character with '-'.
+ Multilib::flags_list PrintOptions;
+ for (StringRef Flag : Flags) {
+if (Flag.front() == '+')
+ PrintOptions.push_back(("-" + Flag.substr(1)).str());
+ }
+ return PrintOptions;
+}
+
+Multilib::flags_list Multilib::getPrintOptions() const {
+ switch (PrintOptions) {
+ case PrintOptionsType::List:
+return PrintOptionsList;
+ case PrintOptionsType::PlusFlags:
+return getPrintOptionsFromPlusFlags(Flags);
+ }
+ llvm_unreachable("Unknown Multilib::PrintOptionsType");
}
LLVM_DUMP_METHOD void Multilib::dump() const {
@@ -44,14 +72,11 @@
void Multilib::print(raw_ostream ) const {
if (GCCSuffix.empty())
OS << ".";
- else {
+ else
OS << StringRef(GCCSuffix).drop_front();
- }
OS << ";";
- for (StringRef Flag : Flags) {
-if (Flag.front() == '+')
- OS << "@" << Flag.substr(1);
- }
+ for (StringRef Option : getPrintOptions())
+OS << "@" << Option.substr(1);
}
bool Multilib::operator==(const Multilib ) const {
Index: clang/include/clang/Driver/Multilib.h
===
--- clang/include/clang/Driver/Multilib.h
+++ clang/include/clang/Driver/Multilib.h
@@ -31,19 +31,39 @@
public:
using flags_list = std::vector;
+ /// When -print-multi-lib is used, this defines how the printed options must
+ /// be calculated.
+ enum class PrintOptionsType {
+/// Use the options stored in
[PATCH] D146757: [Driver] Enable defining multilib print options independently of flags
peter.smith added a comment.
Approach looks good to me. Some suggestions, mostly around documenting the
interface.
Comment at: clang/include/clang/Driver/Multilib.h:56
/// This is enforced with an assert in the constructor.
Multilib(StringRef GCCSuffix = {}, StringRef OSSuffix = {},
+ StringRef IncludeSuffix = {}, const flags_list =
flags_list(),
I think it is worth adding some information from the commit message to the
comment. In particular what the default behaviour is and what, if any,
restrictions are there on the PrintOptionsList.
For example there is a comment in the assert in the constructor body that
probably ought to be in the header
```
// If PrintOptions is something other than List then PrintOptionsList must be
// empty.
```
Are there any restrictions on the format of PrintOptionsList for it to print
correctly? For example does each option need to be prefixed with `-`?
Comment at: clang/include/clang/Driver/Multilib.h:58
+ StringRef IncludeSuffix = {}, const flags_list =
flags_list(),
+ PrintOptionsType PrintOptions = PrintOptionsType::PlusFlags,
+ const flags_list = flags_list());
How would someone using `makeMultilib()` from `MultilibBuilder` use these
parameters? If they can't do they need to? If so we may need something like a
makeMultilib overload to supply the extra parameters.
Comment at: clang/lib/Driver/Multilib.cpp:44
+ assert(PrintOptions == PrintOptionsType::List || PrintOptionsList.empty());
+}
+
If there are any restrictions on the strings in PrintOptionsList could be worth
assertions. Please ignore if there are no restrictions.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146757/new/
https://reviews.llvm.org/D146757
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146757: [Driver] Enable defining multilib print options independently of flags
michaelplatings added a comment. @phosek ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146757/new/ https://reviews.llvm.org/D146757 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146757: [Driver] Enable defining multilib print options independently of flags
michaelplatings created this revision.
michaelplatings added a reviewer: phosek.
Herald added a project: All.
michaelplatings requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.
Sometimes it's necessary to define a multilib in terms of attributes
that don't correspond directly to a valid command line option.
However to support -print-multi-lib we still need to specify a
representative set of command line options for the multilib.
Decoupling flags from print options enables this.
The default of deriving print options from flags beginning '+' remains.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D146757
Files:
clang/include/clang/Driver/Multilib.h
clang/lib/Driver/Multilib.cpp
clang/unittests/Driver/MultilibBuilderTest.cpp
clang/unittests/Driver/MultilibTest.cpp
Index: clang/unittests/Driver/MultilibTest.cpp
===
--- clang/unittests/Driver/MultilibTest.cpp
+++ clang/unittests/Driver/MultilibTest.cpp
@@ -187,3 +187,19 @@
EXPECT_EQ("/a", Selection[0].gccSuffix());
EXPECT_EQ("/b", Selection[1].gccSuffix());
}
+
+TEST(MultilibBuilder, PrintOptions) {
+ ASSERT_EQ(Multilib::flags_list(), Multilib().getPrintOptions());
+ ASSERT_EQ(Multilib::flags_list({"-x"}),
+Multilib({}, {}, {}, {"+x"}).getPrintOptions());
+ ASSERT_EQ(Multilib::flags_list({"-x"}),
+Multilib({}, {}, {}, {"+x"}, Multilib::PrintOptionsType::PlusFlags)
+.getPrintOptions());
+ ASSERT_EQ(
+ Multilib::flags_list({"-x", "-a", "-c"}),
+ Multilib({}, {}, {}, {"+x", "-y", "+a", "-b", "+c"}).getPrintOptions());
+ ASSERT_EQ(
+ Multilib::flags_list({"-y"}),
+ Multilib({}, {}, {}, {"+x"}, Multilib::PrintOptionsType::List, {"-y"})
+ .getPrintOptions());
+}
Index: clang/unittests/Driver/MultilibBuilderTest.cpp
===
--- clang/unittests/Driver/MultilibBuilderTest.cpp
+++ clang/unittests/Driver/MultilibBuilderTest.cpp
@@ -207,3 +207,14 @@
<< "Selection picked " << Selection << " which was not expected ";
}
}
+
+TEST(MultilibBuilderTest, PrintOptions) {
+ Multilib M = MultilibBuilder()
+ .flag("+x")
+ .flag("-y")
+ .flag("+a")
+ .flag("-b")
+ .flag("+c")
+ .makeMultilib();
+ ASSERT_EQ(Multilib::flags_list({"-x", "-a", "-c"}), M.getPrintOptions());
+}
Index: clang/lib/Driver/Multilib.cpp
===
--- clang/lib/Driver/Multilib.cpp
+++ clang/lib/Driver/Multilib.cpp
@@ -26,15 +26,43 @@
using namespace llvm::sys;
Multilib::Multilib(StringRef GCCSuffix, StringRef OSSuffix,
- StringRef IncludeSuffix, const flags_list )
+ StringRef IncludeSuffix, const flags_list ,
+ PrintOptionsType PrintOptions,
+ const flags_list )
: GCCSuffix(GCCSuffix), OSSuffix(OSSuffix), IncludeSuffix(IncludeSuffix),
- Flags(Flags) {
+ Flags(Flags), PrintOptionsList(PrintOptionsList),
+ PrintOptions(PrintOptions) {
assert(GCCSuffix.empty() ||
(StringRef(GCCSuffix).front() == '/' && GCCSuffix.size() > 1));
assert(OSSuffix.empty() ||
(StringRef(OSSuffix).front() == '/' && OSSuffix.size() > 1));
assert(IncludeSuffix.empty() ||
(StringRef(IncludeSuffix).front() == '/' && IncludeSuffix.size() > 1));
+ // If PrintOptions is something other than List then PrintOptionsList must be
+ // empty.
+ assert(PrintOptions == PrintOptionsType::List || PrintOptionsList.empty());
+}
+
+static Multilib::flags_list
+getPrintOptionsFromPlusFlags(const Multilib::flags_list ) {
+ // Derive print options from flags beginning '+', replacing the first
+ // character with '-'.
+ Multilib::flags_list PrintOptions;
+ for (StringRef Flag : Flags) {
+if (Flag.front() == '+')
+ PrintOptions.push_back(("-" + Flag.substr(1)).str());
+ }
+ return PrintOptions;
+}
+
+Multilib::flags_list Multilib::getPrintOptions() const {
+ switch (PrintOptions) {
+ case PrintOptionsType::List:
+return PrintOptionsList;
+ case PrintOptionsType::PlusFlags:
+return getPrintOptionsFromPlusFlags(Flags);
+ }
+ llvm_unreachable("Unknown Multilib::PrintOptionsType");
}
LLVM_DUMP_METHOD void Multilib::dump() const {
@@ -44,14 +72,11 @@
void Multilib::print(raw_ostream ) const {
if (GCCSuffix.empty())
OS << ".";
- else {
+ else
OS << StringRef(GCCSuffix).drop_front();
- }
OS << ";";
- for (StringRef Flag : Flags) {
-if (Flag.front() == '+')
- OS << "@" << Flag.substr(1);
- }
+ for (StringRef Option : getPrintOptions())
+OS << "@" << Option.substr(1);
}
bool Multilib::operator==(const Multilib ) const {
Index: clang/include/clang/Driver/Multilib.h
