subject:"\[PATCH\] D151437\: \[NFC\]\[Driver\] Change MultilibBuilder interface"

[PATCH] D151437: [NFC][Driver] Change MultilibBuilder interface

2023-06-07 Thread Michael Platings via Phabricator via cfe-commits

michaelplatings marked an inline comment as done.
michaelplatings added inline comments.



Comment at: clang/include/clang/Driver/MultilibBuilder.h:80
+  /// \p Flag must be a flag accepted by the driver.
+  MultilibBuilder (bool Required, StringRef Flag);
 

phosek wrote:
> I think it would be cleaner to swap the order of arguments and give the 
> boolean argument a default value. When setting the optional argument, ideally 
> we would always use put the argument in the comment which is a standard 
> convention in LLVM.
> 
> I also think that `Required` is not the best name because it might suggest 
> that `Required=false` means that the argument is optional. A better name 
> might be something like `Negate` or `Disallow` which would also match the `!` 
> notation.
> 
> Here's a concrete example of what I have in mind:
> 
> ```
>   Multilibs.push_back(MultilibBuilder("asan+noexcept", {}, {})
>   .flag("-fsanitize=address")
>   .flag("-fexceptions", /*Negate=*/false)
>   .flag("-fno-exceptions")
>   .makeMultilib());
> ```
OK, I've created D152353

`/*Negate=*/false` would mean the flag is not negated so I've changed that to 
`/*Negate=*/true`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151437

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151437: [NFC][Driver] Change MultilibBuilder interface

2023-06-06 Thread Petr Hosek via Phabricator via cfe-commits

phosek added inline comments.



Comment at: clang/include/clang/Driver/MultilibBuilder.h:80
+  /// \p Flag must be a flag accepted by the driver.
+  MultilibBuilder (bool Required, StringRef Flag);
 

I think it would be cleaner to swap the order of arguments and give the boolean 
argument a default value. When setting the optional argument, ideally we would 
always use put the argument in the comment which is a standard convention in 
LLVM.

I also think that `Required` is not the best name because it might suggest that 
`Required=false` means that the argument is optional. A better name might be 
something like `Negate` or `Disallow` which would also match the `!` notation.

Here's a concrete example of what I have in mind:

```
  Multilibs.push_back(MultilibBuilder("asan+noexcept", {}, {})
  .flag("-fsanitize=address")
  .flag("-fexceptions", /*Negate=*/false)
  .flag("-fno-exceptions")
  .makeMultilib());
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151437

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151437: [NFC][Driver] Change MultilibBuilder interface

2023-06-06 Thread Michael Platings via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG47b431d6617d: [NFC][Driver] Change MultilibBuilder interface 
(authored by michaelplatings).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151437

Files:
  clang/include/clang/Driver/MultilibBuilder.h
  clang/lib/Driver/MultilibBuilder.cpp
  clang/lib/Driver/ToolChains/BareMetal.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Fuchsia.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/OHOS.cpp
  clang/unittests/Driver/MultilibBuilderTest.cpp

Index: clang/unittests/Driver/MultilibBuilderTest.cpp
===
--- clang/unittests/Driver/MultilibBuilderTest.cpp
+++ clang/unittests/Driver/MultilibBuilderTest.cpp
@@ -27,20 +27,22 @@
 
   ASSERT_TRUE(MultilibBuilder().isValid()) << "Empty multilib is not valid";
 
-  ASSERT_TRUE(MultilibBuilder().flag("+foo").isValid())
+  ASSERT_TRUE(MultilibBuilder().flag(true, "-foo").isValid())
   << "Single indicative flag is not valid";
 
-  ASSERT_TRUE(MultilibBuilder().flag("-foo").isValid())
+  ASSERT_TRUE(MultilibBuilder().flag(false, "-foo").isValid())
   << "Single contraindicative flag is not valid";
 
-  ASSERT_FALSE(MultilibBuilder().flag("+foo").flag("-foo").isValid())
+  ASSERT_FALSE(
+  MultilibBuilder().flag(true, "-foo").flag(false, "-foo").isValid())
   << "Conflicting flags should invalidate the Multilib";
 
-  ASSERT_TRUE(MultilibBuilder().flag("+foo").flag("+foo").isValid())
+  ASSERT_TRUE(MultilibBuilder().flag(true, "-foo").flag(true, "-foo").isValid())
   << "Multilib should be valid even if it has the same flag "
  "twice";
 
-  ASSERT_TRUE(MultilibBuilder().flag("+foo").flag("-foobar").isValid())
+  ASSERT_TRUE(
+  MultilibBuilder().flag(true, "-foo").flag(false, "-foobar").isValid())
   << "Seemingly conflicting prefixes shouldn't actually conflict";
 }
 
@@ -52,7 +54,8 @@
 }
 
 TEST(MultilibBuilderTest, Construction3) {
-  MultilibBuilder M = MultilibBuilder().flag("+f1").flag("+f2").flag("-f3");
+  MultilibBuilder M =
+  MultilibBuilder().flag(true, "-f1").flag(true, "-f2").flag(false, "-f3");
   for (const std::string  : M.flags()) {
 ASSERT_TRUE(llvm::StringSwitch(A)
 .Cases("+f1", "+f2", "-f3", true)
@@ -63,7 +66,7 @@
 TEST(MultilibBuilderTest, SetConstruction1) {
   // Single maybe
   MultilibSet MS = MultilibSetBuilder()
-   .Maybe(MultilibBuilder("64").flag("+m64"))
+   .Maybe(MultilibBuilder("64").flag(true, "-m64"))
.makeMultilibSet();
   ASSERT_TRUE(MS.size() == 2);
   for (MultilibSet::const_iterator I = MS.begin(), E = MS.end(); I != E; ++I) {
@@ -79,8 +82,8 @@
 TEST(MultilibBuilderTest, SetConstruction2) {
   // Double maybe
   MultilibSet MS = MultilibSetBuilder()
-   .Maybe(MultilibBuilder("sof").flag("+sof"))
-   .Maybe(MultilibBuilder("el").flag("+EL"))
+   .Maybe(MultilibBuilder("sof").flag(true, "-sof"))
+   .Maybe(MultilibBuilder("el").flag(true, "-EL"))
.makeMultilibSet();
   ASSERT_TRUE(MS.size() == 4);
   for (MultilibSet::const_iterator I = MS.begin(), E = MS.end(); I != E; ++I) {
@@ -154,7 +157,7 @@
 
 TEST(MultilibBuilderTest, SetSelection1) {
   MultilibSet MS1 = MultilibSetBuilder()
-.Maybe(MultilibBuilder("64").flag("+m64"))
+.Maybe(MultilibBuilder("64").flag(true, "-m64"))
 .makeMultilibSet();
 
   Multilib::flags_list FlagM64 = {"+m64"};
@@ -174,8 +177,8 @@
 
 TEST(MultilibBuilderTest, SetSelection2) {
   MultilibSet MS2 = MultilibSetBuilder()
-.Maybe(MultilibBuilder("el").flag("+EL"))
-.Maybe(MultilibBuilder("sf").flag("+SF"))
+.Maybe(MultilibBuilder("el").flag(true, "-EL"))
+.Maybe(MultilibBuilder("sf").flag(true, "-SF"))
 .makeMultilibSet();
 
   for (unsigned I = 0; I < 4; ++I) {
Index: clang/lib/Driver/ToolChains/OHOS.cpp
===
--- clang/lib/Driver/ToolChains/OHOS.cpp
+++ clang/lib/Driver/ToolChains/OHOS.cpp
@@ -66,20 +66,20 @@
   bool IsA7 = false;
   if (const Arg *A = Args.getLastArg(options::OPT_mcpu_EQ))
 IsA7 = A->getValue() == StringRef("cortex-a7");
-  addMultilibFlag(IsA7, "mcpu=cortex-a7", Flags);
+  addMultilibFlag(IsA7, "-mcpu=cortex-a7", Flags);
 
   bool IsMFPU = false;
   if (const Arg *A = Args.getLastArg(options::OPT_mfpu_EQ))
 IsMFPU = A->getValue() == StringRef("neon-vfpv4");
-  addMultilibFlag(IsMFPU, "mfpu=neon-vfpv4", Flags);
+  addMultilibFlag(IsMFPU, "-mfpu=neon-vfpv4", Flags);

[PATCH] D151437: [NFC][Driver] Change MultilibBuilder interface

2023-06-05 Thread Simon Tatham via Phabricator via cfe-commits

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

LGTM: I agree that the new notation is easier to understand for anyone not 
already used to the old one.

However, please wait at least a day for other people and time zones to have a 
chance to respond.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151437

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151437: [NFC][Driver] Change MultilibBuilder interface

2023-05-25 Thread Michael Platings via Phabricator via cfe-commits

michaelplatings created this revision.
michaelplatings added a reviewer: phosek.
Herald added a subscriber: abrachet.
Herald added a project: All.
michaelplatings requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

Decouple the interface of the MultilibBuilder flag method from how flags
are stored internally. Likewise change the addMultilibFlag function.

Currently a multilib flag like "-fexceptions" means a multilib is
*incompatible* with the -fexceptions command line option, which is
counter-intuitive. This change is a step towards changing this scheme.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151437

Files:
  clang/include/clang/Driver/MultilibBuilder.h
  clang/lib/Driver/MultilibBuilder.cpp
  clang/lib/Driver/ToolChains/BareMetal.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Fuchsia.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/OHOS.cpp
  clang/unittests/Driver/MultilibBuilderTest.cpp

Index: clang/unittests/Driver/MultilibBuilderTest.cpp
===
--- clang/unittests/Driver/MultilibBuilderTest.cpp
+++ clang/unittests/Driver/MultilibBuilderTest.cpp
@@ -27,20 +27,22 @@
 
   ASSERT_TRUE(MultilibBuilder().isValid()) << "Empty multilib is not valid";
 
-  ASSERT_TRUE(MultilibBuilder().flag("+foo").isValid())
+  ASSERT_TRUE(MultilibBuilder().flag(true, "-foo").isValid())
   << "Single indicative flag is not valid";
 
-  ASSERT_TRUE(MultilibBuilder().flag("-foo").isValid())
+  ASSERT_TRUE(MultilibBuilder().flag(false, "-foo").isValid())
   << "Single contraindicative flag is not valid";
 
-  ASSERT_FALSE(MultilibBuilder().flag("+foo").flag("-foo").isValid())
+  ASSERT_FALSE(
+  MultilibBuilder().flag(true, "-foo").flag(false, "-foo").isValid())
   << "Conflicting flags should invalidate the Multilib";
 
-  ASSERT_TRUE(MultilibBuilder().flag("+foo").flag("+foo").isValid())
+  ASSERT_TRUE(MultilibBuilder().flag(true, "-foo").flag(true, "-foo").isValid())
   << "Multilib should be valid even if it has the same flag "
  "twice";
 
-  ASSERT_TRUE(MultilibBuilder().flag("+foo").flag("-foobar").isValid())
+  ASSERT_TRUE(
+  MultilibBuilder().flag(true, "-foo").flag(false, "-foobar").isValid())
   << "Seemingly conflicting prefixes shouldn't actually conflict";
 }
 
@@ -52,7 +54,8 @@
 }
 
 TEST(MultilibBuilderTest, Construction3) {
-  MultilibBuilder M = MultilibBuilder().flag("+f1").flag("+f2").flag("-f3");
+  MultilibBuilder M =
+  MultilibBuilder().flag(true, "-f1").flag(true, "-f2").flag(false, "-f3");
   for (const std::string  : M.flags()) {
 ASSERT_TRUE(llvm::StringSwitch(A)
 .Cases("+f1", "+f2", "-f3", true)
@@ -63,7 +66,7 @@
 TEST(MultilibBuilderTest, SetConstruction1) {
   // Single maybe
   MultilibSet MS = MultilibSetBuilder()
-   .Maybe(MultilibBuilder("64").flag("+m64"))
+   .Maybe(MultilibBuilder("64").flag(true, "-m64"))
.makeMultilibSet();
   ASSERT_TRUE(MS.size() == 2);
   for (MultilibSet::const_iterator I = MS.begin(), E = MS.end(); I != E; ++I) {
@@ -79,8 +82,8 @@
 TEST(MultilibBuilderTest, SetConstruction2) {
   // Double maybe
   MultilibSet MS = MultilibSetBuilder()
-   .Maybe(MultilibBuilder("sof").flag("+sof"))
-   .Maybe(MultilibBuilder("el").flag("+EL"))
+   .Maybe(MultilibBuilder("sof").flag(true, "-sof"))
+   .Maybe(MultilibBuilder("el").flag(true, "-EL"))
.makeMultilibSet();
   ASSERT_TRUE(MS.size() == 4);
   for (MultilibSet::const_iterator I = MS.begin(), E = MS.end(); I != E; ++I) {
@@ -154,7 +157,7 @@
 
 TEST(MultilibBuilderTest, SetSelection1) {
   MultilibSet MS1 = MultilibSetBuilder()
-.Maybe(MultilibBuilder("64").flag("+m64"))
+.Maybe(MultilibBuilder("64").flag(true, "-m64"))
 .makeMultilibSet();
 
   Multilib::flags_list FlagM64 = {"+m64"};
@@ -174,8 +177,8 @@
 
 TEST(MultilibBuilderTest, SetSelection2) {
   MultilibSet MS2 = MultilibSetBuilder()
-.Maybe(MultilibBuilder("el").flag("+EL"))
-.Maybe(MultilibBuilder("sf").flag("+SF"))
+.Maybe(MultilibBuilder("el").flag(true, "-EL"))
+.Maybe(MultilibBuilder("sf").flag(true, "-SF"))
 .makeMultilibSet();
 
   for (unsigned I = 0; I < 4; ++I) {
Index: clang/lib/Driver/ToolChains/OHOS.cpp
===
--- clang/lib/Driver/ToolChains/OHOS.cpp
+++ clang/lib/Driver/ToolChains/OHOS.cpp
@@ -66,20 +66,20 @@
   bool IsA7 = false;
   if (const Arg *A = Args.getLastArg(options::OPT_mcpu_EQ))
 IsA7 = A->getValue() ==

[PATCH] D151437: [NFC][Driver] Change MultilibBuilder interface

[PATCH] D151437: [NFC][Driver] Change MultilibBuilder interface

[PATCH] D151437: [NFC][Driver] Change MultilibBuilder interface

[PATCH] D151437: [NFC][Driver] Change MultilibBuilder interface

[PATCH] D151437: [NFC][Driver] Change MultilibBuilder interface

5 matches

Site Navigation

Mail list logo

Footer information