Skip to content

Conversation

@kwk
Copy link
Contributor

@kwk kwk commented Jun 2, 2025

This is a fixup for #141851 and removes = from all
options with additional arguments.

Before 14 out of 22 options with arguments used "=" and 7 didn't.

@kwk kwk requested a review from RoboTux June 2, 2025 07:20
@kwk kwk self-assigned this Jun 2, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 2, 2025

@llvm/pr-subscribers-testing-tools

Author: Konrad Kleine (kwk)

Changes

This is a fixup for #141851.


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

1 Files Affected:

  • (modified) llvm/docs/CommandGuide/lit.rst (+1-1)
diff --git a/llvm/docs/CommandGuide/lit.rst b/llvm/docs/CommandGuide/lit.rst
index 86879f870e06e..7f2ea6b82a716 100644
--- a/llvm/docs/CommandGuide/lit.rst
+++ b/llvm/docs/CommandGuide/lit.rst
@@ -218,7 +218,7 @@ EXECUTION OPTIONS
 
  Stop execution after the given number of failures.
 
-.. option:: --max-retries-per-test N
+.. option:: --max-retries-per-test=N
 
  Retry running failed tests at most ``N`` times.
  Out of the following options to rerun failed tests the

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

It seems like the docs use a mix of both styles -- should we normalize to one or the other for all options?

@kwk kwk changed the title [docs] use = in --max-retries-per-test=N [docs] use "=" in options with arguments Jun 2, 2025
@kwk
Copy link
Contributor Author

kwk commented Jun 2, 2025

It seems like the docs use a mix of both styles -- should we normalize to one or the other for all options?

Oh, indeed. I must say in terms of readability I like the one without the = better.

Here are some stats:

  • There are 53 options in the file
    • grep '.. option::' llvm/docs/CommandGuide/lit.rst | wc -l
  • There are 22 options with additional arguments (e.g. in capital letters)
    • grep '.. option::' llvm/docs/CommandGuide/lit.rst | grep -P '[A-Z=]' | wc -l
  • Out of the 22 options with arguments, there are 14 options that use "=" and 7 that use no "=".
    • grep '.. option::' llvm/docs/CommandGuide/lit.rst | grep -P '[A-Z]' | grep "=" | wc -l
    • grep '.. option::' llvm/docs/CommandGuide/lit.rst | grep -P '[A-Z]' | grep -v "=" | wc -l

That means the majority of options with additional arguments uses =.

Consistency wise we should probably use = for this PR and the rest as well. I've changed this PR accordingly.

@kwk kwk force-pushed the fixup-for-141851 branch from f72b7a8 to 9432d15 Compare June 2, 2025 10:41
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

The print text that lit itself prints uses whitespace instead of =, so I'd be inclined to normalize the other way around...

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, -o=PATH looks quite unusual.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does.

@kwk
Copy link
Contributor Author

kwk commented Jun 2, 2025

The print test that lit itself prints uses whitespace instead of =, so I'd be inclined to normalize the other way around...

Like I said, this would have been my favourable way too but I went with the majority of what was in the file. Let me work on this real quick.

@kwk kwk force-pushed the fixup-for-141851 branch from 9432d15 to aa68d4e Compare June 2, 2025 15:37
@kwk kwk changed the title [docs] use "=" in options with arguments [docs] don't use "=" in lit options with arguments Jun 2, 2025
@kwk
Copy link
Contributor Author

kwk commented Jun 2, 2025

@nikic can you recheck?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change correct? I think = here is part of the parameter argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am a lousy programmer it seems. But luckily you're a great reviewer. You're absolutely right. This HAS to stay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixed now.

This is a fixup for llvm#141851 and removes `=` from all
options with additional arguments.

Before 14 out of 22 options with arguments used "=" and 7 didn't.
@kwk kwk force-pushed the fixup-for-141851 branch from aa68d4e to 0d28919 Compare June 4, 2025 07:00
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM, this matches llvm-lit --help.

@kwk kwk merged commit 107d8c7 into llvm:main Jun 4, 2025
8 of 10 checks passed
@kwk kwk deleted the fixup-for-141851 branch June 4, 2025 10:36
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