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] Add quotes around option name #81784

Merged
merged 1 commit into from
Feb 15, 2024
Merged

Conversation

lamb-j
Copy link
Contributor

@lamb-j lamb-j commented Feb 14, 2024

No description provided.

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 14, 2024

@llvm/pr-subscribers-llvm-support

Author: Jacob Lambert (lamb-j)

Changes

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

1 Files Affected:

  • (modified) llvm/include/llvm/Support/CommandLine.h (+1-1)
diff --git a/llvm/include/llvm/Support/CommandLine.h b/llvm/include/llvm/Support/CommandLine.h
index 8929f9b1db15c7..99dc9aefbd7d63 100644
--- a/llvm/include/llvm/Support/CommandLine.h
+++ b/llvm/include/llvm/Support/CommandLine.h
@@ -878,7 +878,7 @@ template <class DataType> class parser : public generic_parser_base {
   void addLiteralOption(StringRef Name, const DT &V, StringRef HelpStr) {
 #ifndef NDEBUG
     if (findOption(Name) != Values.size())
-      report_fatal_error("Option " + Name + " already exists!");
+      report_fatal_error("Option '" + Name + "' already exists!");
 #endif
     OptionInfo X(Name, static_cast<DataType>(V), HelpStr);
     Values.push_back(X);

@makslevental
Copy link
Contributor

@lamb-j let me know if you need me to merge.

@lamb-j lamb-j merged commit 5375009 into llvm:main Feb 15, 2024
6 checks passed
@dwblaikie
Copy link
Collaborator

Could you add some test coverage for this?

@makslevental-amd
Copy link

makslevental-amd commented Feb 15, 2024

You can't? You'd have to actually register overlapping command line options which would fatal_error for all the other tests too?

@lamb-j
Copy link
Contributor Author

lamb-j commented Feb 15, 2024

We may be able to create a small reproducer that defines two overlapping options, such that when it's compiled an executed it produces the error? Looking into it now.

@lamb-j lamb-j deleted the quote-option branch February 15, 2024 18:29
@dwblaikie
Copy link
Collaborator

Oh, sorry, I see it's a report_fatal_error - just thought it was a normal error message, not an assert/fatal/library-misuse. Sometimes we test those (like llvm::Error has tests to ensure it assert-fails in the right places/ways) but for one that's only reachable through a custom binary, not sure it'll be worth the hassle. Appreciate you looking into it, though - maybe there's a way that's not so bad.

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.

None yet

5 participants