Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[llvm][support] Show name of overlapping cl option #70108

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

makslevental
Copy link
Contributor

@makslevental makslevental commented Oct 24, 2023

No description provided.

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 24, 2023

@llvm/pr-subscribers-llvm-support

Author: Maksim Levental (makslevental)

Changes

Base commit adds Name to the assert if there's an overlapping cl opt:

   template <class DT>
   void addLiteralOption(StringRef Name, const DT &V, StringRef HelpStr) {
-    assert(findOption(Name) == Values.size() && "Option already exists!");
+    assert(findOption(Name) == Values.size() &&
+           Twine("Option " + Name + " already exists!"));
     OptionInfo X(Name, static_cast<DataType>(V), HelpStr);
     Values.push_back(X);
     AddLiteralOption(Owner, Name);

cc @zzzDavid


Full diff: https://github.com/llvm/llvm-project/pull/70108.diff

1 Files Affected:

  • (modified) llvm/include/llvm/Support/CommandLine.h (+21-21)
diff --git a/llvm/include/llvm/Support/CommandLine.h b/llvm/include/llvm/Support/CommandLine.h
index 49f4a668ae416fe..11bdb1e833d7df9 100644
--- a/llvm/include/llvm/Support/CommandLine.h
+++ b/llvm/include/llvm/Support/CommandLine.h
@@ -185,8 +185,7 @@ class OptionCategory {
   void registerCategory();
 
 public:
-  OptionCategory(StringRef const Name,
-                 StringRef const Description = "")
+  OptionCategory(StringRef const Name, StringRef const Description = "")
       : Name(Name), Description(Description) {
     registerCategory();
   }
@@ -212,7 +211,7 @@ class SubCommand {
 public:
   SubCommand(StringRef Name, StringRef Description = "")
       : Name(Name), Description(Description) {
-        registerSubCommand();
+    registerSubCommand();
   }
   SubCommand() = default;
 
@@ -393,7 +392,8 @@ class Option {
                              bool MultiArg = false);
 
   // Prints option name followed by message.  Always returns true.
-  bool error(const Twine &Message, StringRef ArgName = StringRef(), raw_ostream &Errs = llvm::errs());
+  bool error(const Twine &Message, StringRef ArgName = StringRef(),
+             raw_ostream &Errs = llvm::errs());
   bool error(const Twine &Message, raw_ostream &Errs) {
     return error(Message, StringRef(), Errs);
   }
@@ -446,8 +446,7 @@ template <class Ty> initializer<Ty> init(const Ty &Val) {
   return initializer<Ty>(Val);
 }
 
-template <class Ty>
-list_initializer<Ty> list_init(ArrayRef<Ty> Vals) {
+template <class Ty> list_initializer<Ty> list_init(ArrayRef<Ty> Vals) {
   return list_initializer<Ty>(Vals);
 }
 
@@ -502,7 +501,8 @@ template <typename R, typename C, typename... Args>
 struct callback_traits<R (C::*)(Args...) const> {
   using result_type = R;
   using arg_type = std::tuple_element_t<0, std::tuple<Args...>>;
-  static_assert(sizeof...(Args) == 1, "callback function must have one and only one parameter");
+  static_assert(sizeof...(Args) == 1,
+                "callback function must have one and only one parameter");
   static_assert(std::is_same_v<result_type, void>,
                 "callback return type must be void");
   static_assert(std::is_lvalue_reference_v<arg_type> &&
@@ -528,7 +528,7 @@ struct GenericOptionValue {
 
 protected:
   GenericOptionValue() = default;
-  GenericOptionValue(const GenericOptionValue&) = default;
+  GenericOptionValue(const GenericOptionValue &) = default;
   GenericOptionValue &operator=(const GenericOptionValue &) = default;
   ~GenericOptionValue() = default;
 
@@ -569,7 +569,7 @@ template <class DataType> class OptionValueCopy : public GenericOptionValue {
   bool Valid = false;
 
 protected:
-  OptionValueCopy(const OptionValueCopy&) = default;
+  OptionValueCopy(const OptionValueCopy &) = default;
   OptionValueCopy &operator=(const OptionValueCopy &) = default;
   ~OptionValueCopy() = default;
 
@@ -607,7 +607,7 @@ struct OptionValueBase<DataType, false> : OptionValueCopy<DataType> {
 
 protected:
   OptionValueBase() = default;
-  OptionValueBase(const OptionValueBase&) = default;
+  OptionValueBase(const OptionValueBase &) = default;
   OptionValueBase &operator=(const OptionValueBase &) = default;
   ~OptionValueBase() = default;
 };
@@ -863,7 +863,8 @@ template <class DataType> class parser : public generic_parser_base {
   ///
   template <class DT>
   void addLiteralOption(StringRef Name, const DT &V, StringRef HelpStr) {
-    assert(findOption(Name) == Values.size() && "Option already exists!");
+    assert(findOption(Name) == Values.size() &&
+           Twine("Option " + Name + " already exists!"));
     OptionInfo X(Name, static_cast<DataType>(V), HelpStr);
     Values.push_back(X);
     AddLiteralOption(Owner, Name);
@@ -1263,7 +1264,7 @@ template <unsigned n> struct applicator<const char[n]> {
     O.setArgStr(Str);
   }
 };
-template <> struct applicator<StringRef > {
+template <> struct applicator<StringRef> {
   template <class Opt> static void opt(StringRef Str, Opt &O) {
     O.setArgStr(Str);
   }
@@ -1297,7 +1298,7 @@ template <> struct applicator<MiscFlags> {
 
 // Apply modifiers to an option in a type safe way.
 template <class Opt, class Mod, class... Mods>
-void apply(Opt *O, const Mod &M, const Mods &... Ms) {
+void apply(Opt *O, const Mod &M, const Mods &...Ms) {
   applicator<Mod>::opt(M, *O);
   apply(O, Ms...);
 }
@@ -1486,7 +1487,7 @@ class opt
   }
 
   template <class... Mods>
-  explicit opt(const Mods &... Ms)
+  explicit opt(const Mods &...Ms)
       : Option(llvm::cl::Optional, NotHidden), Parser(*this) {
     apply(this, Ms...);
     done();
@@ -1587,9 +1588,7 @@ template <class DataType> class list_storage<DataType, bool> {
   reference operator[](size_type pos) { return Storage[pos]; }
   const_reference operator[](size_type pos) const { return Storage[pos]; }
 
-  void clear() {
-    Storage.clear();
-  }
+  void clear() { Storage.clear(); }
 
   iterator erase(const_iterator pos) { return Storage.erase(pos); }
   iterator erase(const_iterator first, const_iterator last) {
@@ -1726,7 +1725,7 @@ class list : public Option, public list_storage<DataType, StorageClass> {
   void setNumAdditionalVals(unsigned n) { Option::setNumAdditionalVals(n); }
 
   template <class... Mods>
-  explicit list(const Mods &... Ms)
+  explicit list(const Mods &...Ms)
       : Option(ZeroOrMore, NotHidden), Parser(*this) {
     apply(this, Ms...);
     done();
@@ -1882,7 +1881,7 @@ class bits : public Option, public bits_storage<DataType, Storage> {
   }
 
   template <class... Mods>
-  explicit bits(const Mods &... Ms)
+  explicit bits(const Mods &...Ms)
       : Option(ZeroOrMore, NotHidden), Parser(*this) {
     apply(this, Ms...);
     done();
@@ -1934,7 +1933,8 @@ class alias : public Option {
     if (!AliasFor)
       error("cl::alias must have an cl::aliasopt(option) specified!");
     if (!Subs.empty())
-      error("cl::alias must not have cl::sub(), aliased option's cl::sub() will be used!");
+      error("cl::alias must not have cl::sub(), aliased option's cl::sub() "
+            "will be used!");
     Subs = AliasFor->Subs;
     Categories = AliasFor->Categories;
     addArgument();
@@ -1952,7 +1952,7 @@ class alias : public Option {
   }
 
   template <class... Mods>
-  explicit alias(const Mods &... Ms)
+  explicit alias(const Mods &...Ms)
       : Option(Optional, Hidden), AliasFor(nullptr) {
     apply(this, Ms...);
     done();

@makslevental makslevental force-pushed the cl_opt_name branch 3 times, most recently from 6faf98b to d0bfb7a Compare October 24, 2023 19:57
@aeubanks
Copy link
Contributor

makes sense in concept, but an assert is not going to print out the name, it'll literally print out the source code within the assert. should have something like

#ifndef NDEBUG
if (...) {
  report_fatal_error(Name + " already registered", false);
}

instead which will actually print out the name

also please split out the formatting changes, can push that directly without review

@makslevental
Copy link
Contributor Author

instead which will actually print out the name

D'oh - I knew there must be some reason it wasn't parameterized in the first place but couldn't figure out exactly why. Okay I'll try with the #ifndef (and I'll split out the formatting).

Copy link
Contributor

@aeubanks aeubanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some nits, otherwise lgtm

llvm/include/llvm/Support/CommandLine.h Outdated Show resolved Hide resolved
llvm/include/llvm/Support/CommandLine.h Outdated Show resolved Hide resolved
Copy link
Contributor

@aeubanks aeubanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, but change the commit message before pushing

@makslevental
Copy link
Contributor Author

lgtm, but change the commit message before pushing

Sure - what should it be?

@aeubanks
Copy link
Contributor

honestly the title itself is descriptive enough, I'd just remove the description

@makslevental makslevental merged commit 0555c9a into llvm:main Oct 25, 2023
3 checks passed
@makslevental makslevental deleted the cl_opt_name branch October 25, 2023 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants