[PATCH] D142893: [NFC] Class for building MultilibSet

[Michael Platings via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
michaelplatings marked 4 inline comments as done.
Closed by commit rG850dab0f2537: [NFC] Class for building MultilibSet (authored 
by michaelplatings).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142893/new/

https://reviews.llvm.org/D142893

Files:
  clang/include/clang/Driver/Multilib.h
  clang/include/clang/Driver/MultilibBuilder.h
  clang/lib/Driver/CMakeLists.txt
  clang/lib/Driver/Multilib.cpp
  clang/lib/Driver/MultilibBuilder.cpp
  clang/lib/Driver/ToolChains/BareMetal.cpp
  clang/lib/Driver/ToolChains/Fuchsia.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/unittests/Driver/CMakeLists.txt
  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
@@ -11,34 +11,17 @@
 //===--===//
 
 #include "clang/Driver/Multilib.h"
+#include "../../lib/Driver/ToolChains/CommonArgs.h"
 #include "clang/Basic/LLVM.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/SourceMgr.h"
 #include "gtest/gtest.h"
 
 using namespace clang::driver;
 using namespace clang;
 
-TEST(MultilibTest, MultilibValidity) {
-
-  ASSERT_TRUE(Multilib().isValid()) << "Empty multilib is not valid";
-
-  ASSERT_TRUE(Multilib().flag("+foo").isValid())
-  << "Single indicative flag is not valid";
-
-  ASSERT_TRUE(Multilib().flag("-foo").isValid())
-  << "Single contraindicative flag is not valid";
-
-  ASSERT_FALSE(Multilib().flag("+foo").flag("-foo").isValid())
-  << "Conflicting flags should invalidate the Multilib";
-
-  ASSERT_TRUE(Multilib().flag("+foo").flag("+foo").isValid())
-  << "Multilib should be valid even if it has the same flag twice";
-
-  ASSERT_TRUE(Multilib().flag("+foo").flag("-foobar").isValid())
-  << "Seemingly conflicting prefixes shouldn't actually conflict";
-}
-
 TEST(MultilibTest, OpEqReflexivity1) {
   Multilib M;
   ASSERT_TRUE(M == M) << "Multilib::operator==() is not reflexive";
@@ -50,40 +33,28 @@
 }
 
 TEST(MultilibTest, OpEqReflexivity3) {
-  Multilib M1, M2;
-  M1.flag("+foo");
-  M2.flag("+foo");
+  Multilib M1({}, {}, {}, 0, {"+foo"});
+  Multilib M2({}, {}, {}, 0, {"+foo"});
   ASSERT_TRUE(M1 == M2) << "Multilibs with the same flag should be the same";
 }
 
 TEST(MultilibTest, OpEqInequivalence1) {
-  Multilib M1, M2;
-  M1.flag("+foo");
-  M2.flag("-foo");
+  Multilib M1({}, {}, {}, 0, {"+foo"});
+  Multilib M2({}, {}, {}, 0, {"-foo"});
   ASSERT_FALSE(M1 == M2) << "Multilibs with conflicting flags are not the same";
   ASSERT_FALSE(M2 == M1)
   << "Multilibs with conflicting flags are not the same (commuted)";
 }
 
 TEST(MultilibTest, OpEqInequivalence2) {
-  Multilib M1, M2;
-  M2.flag("+foo");
+  Multilib M1;
+  Multilib M2({}, {}, {}, 0, {"+foo"});
   ASSERT_FALSE(M1 == M2) << "Flags make Multilibs different";
 }
 
-TEST(MultilibTest, OpEqEquivalence1) {
-  Multilib M1, M2;
-  M1.flag("+foo");
-  M2.flag("+foo").flag("+foo");
-  ASSERT_TRUE(M1 == M2) << "Flag duplication shouldn't affect equivalence";
-  ASSERT_TRUE(M2 == M1)
-  << "Flag duplication shouldn't affect equivalence (commuted)";
-}
-
 TEST(MultilibTest, OpEqEquivalence2) {
-  Multilib M1("64");
-  Multilib M2;
-  M2.gccSuffix("/64");
+  Multilib M1("/64");
+  Multilib M2("/64");
   ASSERT_TRUE(M1 == M2)
   << "Constructor argument must match Multilib::gccSuffix()";
   ASSERT_TRUE(M2 == M1)
@@ -91,9 +62,8 @@
 }
 
 TEST(MultilibTest, OpEqEquivalence3) {
-  Multilib M1("", "32");
-  Multilib M2;
-  M2.osSuffix("/32");
+  Multilib M1("", "/32");
+  Multilib M2("", "/32");
   ASSERT_TRUE(M1 == M2)
   << "Constructor argument must match Multilib::osSuffix()";
   ASSERT_TRUE(M2 == M1)
@@ -101,9 +71,8 @@
 }
 
 TEST(MultilibTest, OpEqEquivalence4) {
-  Multilib M1("", "", "16");
-  Multilib M2;
-  M2.includeSuffix("/16");
+  Multilib M1("", "", "/16");
+  Multilib M2("", "", "/16");
   ASSERT_TRUE(M1 == M2)
   << "Constructor argument must match Multilib::includeSuffix()";
   ASSERT_TRUE(M2 == M1)
@@ -111,31 +80,31 @@
 }
 
 TEST(MultilibTest, OpEqInequivalence3) {
-  Multilib M1("foo");
-  Multilib M2("bar");
+  Multilib M1("/foo");
+  Multilib M2("/bar");
   ASSERT_FALSE(M1 == M2) << "Differing gccSuffixes should be different";
   ASSERT_FALSE(M2 == M1)
   << "Differing gccSuffixes should be different (commuted)";
 }
 
 TEST(MultilibTest, OpEqInequivalence4) {
-  Multilib M1("", "foo");
-  Multilib M2("", "bar");
+  Multilib M1("", "/foo");
+  Multilib M2("", "/bar");
   ASSERT_FALSE(M1 == M2) << "Differing osSuffixes should be different";
   ASSERT_FALSE(M2 == M1)
   << "Differing osSuffixes should be 

[PATCH] D142893: [NFC] Class for building MultilibSet

[Petr Hosek via Phabricator via cfe-commits]

phosek accepted this revision.
phosek added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang/include/clang/Driver/MultilibBuilder.h:117
+  /// Add an optional Multilib segment
+  MultilibSetBuilder (const MultilibBuilder );
+

MaskRay wrote:
> Use `camelCase` for new function names.
These functions were copies from the existing `MultilibSet` class so I'm fine 
keeping the original names to reduce the amount of changes. If we decide to 
change the names, it would ideally be done in a follow up change.


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

[PATCH] D142893: [NFC] Class for building MultilibSet

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/include/clang/Driver/MultilibBuilder.h:117
+  /// Add an optional Multilib segment
+  MultilibSetBuilder (const MultilibBuilder );
+

Use `camelCase` for new function names.


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

[PATCH] D142893: [NFC] Class for building MultilibSet

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Thanks for the updates, I'm happy with the changes and don't have any more 
comments. Happy to give my implicit approval.


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

[PATCH] D142893: [NFC] Class for building MultilibSet

[Michael Platings via Phabricator via cfe-commits]

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;
   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 (const char *Regex);
-
   /// Add a completed Multilib to the set
   void push_back(const Multilib );
 

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

[PATCH] D142893: [NFC] Class for building MultilibSet

[Michael Platings via Phabricator via cfe-commits]

michaelplatings updated this revision to Diff 495798.
michaelplatings added a comment.

Replace makeMultilibBuilder() with a MultilibBuilder constructor that 
initializes all suffixes to the same value.

In the case of AndroidMipsMultilibs it was apparently intended that only the 
GCC suffix was set, and the other suffixes were left empty. This is now 
explicit in the code.

Comments about Multilib being "immutable" were overstated so I've now removed 
them. The intention is merely to remove the requirement to support mutation. 
The code is simpler when you don't need to keep checking invariants.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142893/new/

https://reviews.llvm.org/D142893

Files:
  clang/include/clang/Driver/Multilib.h
  clang/include/clang/Driver/MultilibBuilder.h
  clang/lib/Driver/CMakeLists.txt
  clang/lib/Driver/Multilib.cpp
  clang/lib/Driver/MultilibBuilder.cpp
  clang/lib/Driver/ToolChains/BareMetal.cpp
  clang/lib/Driver/ToolChains/Fuchsia.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/unittests/Driver/CMakeLists.txt
  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
@@ -11,34 +11,17 @@
 //===--===//
 
 #include "clang/Driver/Multilib.h"
+#include "../../lib/Driver/ToolChains/CommonArgs.h"
 #include "clang/Basic/LLVM.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/SourceMgr.h"
 #include "gtest/gtest.h"
 
 using namespace clang::driver;
 using namespace clang;
 
-TEST(MultilibTest, MultilibValidity) {
-
-  ASSERT_TRUE(Multilib().isValid()) << "Empty multilib is not valid";
-
-  ASSERT_TRUE(Multilib().flag("+foo").isValid())
-  << "Single indicative flag is not valid";
-
-  ASSERT_TRUE(Multilib().flag("-foo").isValid())
-  << "Single contraindicative flag is not valid";
-
-  ASSERT_FALSE(Multilib().flag("+foo").flag("-foo").isValid())
-  << "Conflicting flags should invalidate the Multilib";
-
-  ASSERT_TRUE(Multilib().flag("+foo").flag("+foo").isValid())
-  << "Multilib should be valid even if it has the same flag twice";
-
-  ASSERT_TRUE(Multilib().flag("+foo").flag("-foobar").isValid())
-  << "Seemingly conflicting prefixes shouldn't actually conflict";
-}
-
 TEST(MultilibTest, OpEqReflexivity1) {
   Multilib M;
   ASSERT_TRUE(M == M) << "Multilib::operator==() is not reflexive";
@@ -50,40 +33,28 @@
 }
 
 TEST(MultilibTest, OpEqReflexivity3) {
-  Multilib M1, M2;
-  M1.flag("+foo");
-  M2.flag("+foo");
+  Multilib M1({}, {}, {}, 0, {"+foo"});
+  Multilib M2({}, {}, {}, 0, {"+foo"});
   ASSERT_TRUE(M1 == M2) << "Multilibs with the same flag should be the same";
 }
 
 TEST(MultilibTest, OpEqInequivalence1) {
-  Multilib M1, M2;
-  M1.flag("+foo");
-  M2.flag("-foo");
+  Multilib M1({}, {}, {}, 0, {"+foo"});
+  Multilib M2({}, {}, {}, 0, {"-foo"});
   ASSERT_FALSE(M1 == M2) << "Multilibs with conflicting flags are not the same";
   ASSERT_FALSE(M2 == M1)
   << "Multilibs with conflicting flags are not the same (commuted)";
 }
 
 TEST(MultilibTest, OpEqInequivalence2) {
-  Multilib M1, M2;
-  M2.flag("+foo");
+  Multilib M1;
+  Multilib M2({}, {}, {}, 0, {"+foo"});
   ASSERT_FALSE(M1 == M2) << "Flags make Multilibs different";
 }
 
-TEST(MultilibTest, OpEqEquivalence1) {
-  Multilib M1, M2;
-  M1.flag("+foo");
-  M2.flag("+foo").flag("+foo");
-  ASSERT_TRUE(M1 == M2) << "Flag duplication shouldn't affect equivalence";
-  ASSERT_TRUE(M2 == M1)
-  << "Flag duplication shouldn't affect equivalence (commuted)";
-}
-
 TEST(MultilibTest, OpEqEquivalence2) {
-  Multilib M1("64");
-  Multilib M2;
-  M2.gccSuffix("/64");
+  Multilib M1("/64");
+  Multilib M2("/64");
   ASSERT_TRUE(M1 == M2)
   << "Constructor argument must match Multilib::gccSuffix()";
   ASSERT_TRUE(M2 == M1)
@@ -91,9 +62,8 @@
 }
 
 TEST(MultilibTest, OpEqEquivalence3) {
-  Multilib M1("", "32");
-  Multilib M2;
-  M2.osSuffix("/32");
+  Multilib M1("", "/32");
+  Multilib M2("", "/32");
   ASSERT_TRUE(M1 == M2)
   << "Constructor argument must match Multilib::osSuffix()";
   ASSERT_TRUE(M2 == M1)
@@ -101,9 +71,8 @@
 }
 
 TEST(MultilibTest, OpEqEquivalence4) {
-  Multilib M1("", "", "16");
-  Multilib M2;
-  M2.includeSuffix("/16");
+  Multilib M1("", "", "/16");
+  Multilib M2("", "", "/16");
   ASSERT_TRUE(M1 == M2)
   << "Constructor argument must match Multilib::includeSuffix()";
   ASSERT_TRUE(M2 == M1)
@@ -111,31 +80,31 @@
 }
 
 TEST(MultilibTest, OpEqInequivalence3) {
-  Multilib M1("foo");
-  Multilib M2("bar");
+  Multilib M1("/foo");
+  Multilib M2("/bar");
   ASSERT_FALSE(M1 == M2) << "Differing gccSuffixes should be different";
   

[PATCH] D142893: [NFC] Class for building MultilibSet

[Petr Hosek via Phabricator via cfe-commits]

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;
   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 (const char *Regex);
-
   /// Add a completed Multilib to the set
   void push_back(const Multilib );
 

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

[PATCH] D142893: [NFC] Class for building MultilibSet

[Michael Platings via Phabricator via cfe-commits]

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

[PATCH] D142893: [NFC] Class for building MultilibSet

[Michael Platings via Phabricator via cfe-commits]

michaelplatings updated this revision to Diff 495460.
michaelplatings marked 6 inline comments as done.
michaelplatings added a comment.

Apply improvements suggested by @peter.smith


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142893/new/

https://reviews.llvm.org/D142893

Files:
  clang/include/clang/Driver/Multilib.h
  clang/include/clang/Driver/MultilibBuilder.h
  clang/lib/Driver/CMakeLists.txt
  clang/lib/Driver/Multilib.cpp
  clang/lib/Driver/MultilibBuilder.cpp
  clang/lib/Driver/ToolChains/BareMetal.cpp
  clang/lib/Driver/ToolChains/Fuchsia.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/unittests/Driver/CMakeLists.txt
  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
@@ -11,34 +11,17 @@
 //===--===//
 
 #include "clang/Driver/Multilib.h"
+#include "../../lib/Driver/ToolChains/CommonArgs.h"
 #include "clang/Basic/LLVM.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/SourceMgr.h"
 #include "gtest/gtest.h"
 
 using namespace clang::driver;
 using namespace clang;
 
-TEST(MultilibTest, MultilibValidity) {
-
-  ASSERT_TRUE(Multilib().isValid()) << "Empty multilib is not valid";
-
-  ASSERT_TRUE(Multilib().flag("+foo").isValid())
-  << "Single indicative flag is not valid";
-
-  ASSERT_TRUE(Multilib().flag("-foo").isValid())
-  << "Single contraindicative flag is not valid";
-
-  ASSERT_FALSE(Multilib().flag("+foo").flag("-foo").isValid())
-  << "Conflicting flags should invalidate the Multilib";
-
-  ASSERT_TRUE(Multilib().flag("+foo").flag("+foo").isValid())
-  << "Multilib should be valid even if it has the same flag twice";
-
-  ASSERT_TRUE(Multilib().flag("+foo").flag("-foobar").isValid())
-  << "Seemingly conflicting prefixes shouldn't actually conflict";
-}
-
 TEST(MultilibTest, OpEqReflexivity1) {
   Multilib M;
   ASSERT_TRUE(M == M) << "Multilib::operator==() is not reflexive";
@@ -50,40 +33,28 @@
 }
 
 TEST(MultilibTest, OpEqReflexivity3) {
-  Multilib M1, M2;
-  M1.flag("+foo");
-  M2.flag("+foo");
+  Multilib M1({}, {}, {}, 0, {"+foo"});
+  Multilib M2({}, {}, {}, 0, {"+foo"});
   ASSERT_TRUE(M1 == M2) << "Multilibs with the same flag should be the same";
 }
 
 TEST(MultilibTest, OpEqInequivalence1) {
-  Multilib M1, M2;
-  M1.flag("+foo");
-  M2.flag("-foo");
+  Multilib M1({}, {}, {}, 0, {"+foo"});
+  Multilib M2({}, {}, {}, 0, {"-foo"});
   ASSERT_FALSE(M1 == M2) << "Multilibs with conflicting flags are not the same";
   ASSERT_FALSE(M2 == M1)
   << "Multilibs with conflicting flags are not the same (commuted)";
 }
 
 TEST(MultilibTest, OpEqInequivalence2) {
-  Multilib M1, M2;
-  M2.flag("+foo");
+  Multilib M1;
+  Multilib M2({}, {}, {}, 0, {"+foo"});
   ASSERT_FALSE(M1 == M2) << "Flags make Multilibs different";
 }
 
-TEST(MultilibTest, OpEqEquivalence1) {
-  Multilib M1, M2;
-  M1.flag("+foo");
-  M2.flag("+foo").flag("+foo");
-  ASSERT_TRUE(M1 == M2) << "Flag duplication shouldn't affect equivalence";
-  ASSERT_TRUE(M2 == M1)
-  << "Flag duplication shouldn't affect equivalence (commuted)";
-}
-
 TEST(MultilibTest, OpEqEquivalence2) {
-  Multilib M1("64");
-  Multilib M2;
-  M2.gccSuffix("/64");
+  Multilib M1("/64");
+  Multilib M2("/64");
   ASSERT_TRUE(M1 == M2)
   << "Constructor argument must match Multilib::gccSuffix()";
   ASSERT_TRUE(M2 == M1)
@@ -91,9 +62,8 @@
 }
 
 TEST(MultilibTest, OpEqEquivalence3) {
-  Multilib M1("", "32");
-  Multilib M2;
-  M2.osSuffix("/32");
+  Multilib M1("", "/32");
+  Multilib M2("", "/32");
   ASSERT_TRUE(M1 == M2)
   << "Constructor argument must match Multilib::osSuffix()";
   ASSERT_TRUE(M2 == M1)
@@ -101,9 +71,8 @@
 }
 
 TEST(MultilibTest, OpEqEquivalence4) {
-  Multilib M1("", "", "16");
-  Multilib M2;
-  M2.includeSuffix("/16");
+  Multilib M1("", "", "/16");
+  Multilib M2("", "", "/16");
   ASSERT_TRUE(M1 == M2)
   << "Constructor argument must match Multilib::includeSuffix()";
   ASSERT_TRUE(M2 == M1)
@@ -111,31 +80,31 @@
 }
 
 TEST(MultilibTest, OpEqInequivalence3) {
-  Multilib M1("foo");
-  Multilib M2("bar");
+  Multilib M1("/foo");
+  Multilib M2("/bar");
   ASSERT_FALSE(M1 == M2) << "Differing gccSuffixes should be different";
   ASSERT_FALSE(M2 == M1)
   << "Differing gccSuffixes should be different (commuted)";
 }
 
 TEST(MultilibTest, OpEqInequivalence4) {
-  Multilib M1("", "foo");
-  Multilib M2("", "bar");
+  Multilib M1("", "/foo");
+  Multilib M2("", "/bar");
   ASSERT_FALSE(M1 == M2) << "Differing osSuffixes should be different";
   ASSERT_FALSE(M2 == M1)
   << "Differing osSuffixes should be different (commuted)";
 }
 
 

[PATCH] D142893: [NFC] Class for building MultilibSet

[Peter Smith via Phabricator via cfe-commits]

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