Skip to content

[optionParser] mv to builtins dir - #23037

Open
ferdymercury wants to merge 2 commits into
root-project:masterfrom
ferdymercury:patch-24
Open

[optionParser] mv to builtins dir#23037
ferdymercury wants to merge 2 commits into
root-project:masterfrom
ferdymercury:patch-24

Conversation

@ferdymercury

@ferdymercury ferdymercury commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

This Pull request:

Changes or fixes:

Move this to hard-coded builtin since it's forked from 1.3 and at some point we might resync with upstream 1.7
so having it in builtins is more consistent with other builtins such as mathtext, etc. rather than having it lost somewhere inside.

to show that it's an external source and to potentially later resync with upstream 1.7 rather than current 1.3
@ferdymercury ferdymercury added the skip code analysis Skip the code analysis CI steps for this PR, including verifying clang-formatting and running Ruff. label Aug 6, 2026
@ferdymercury
ferdymercury requested a review from silverweed August 6, 2026 11:28
@ferdymercury
ferdymercury marked this pull request as ready for review August 6, 2026 11:29
@silverweed

silverweed commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

at some point we might resync with upstream 1.7

Should we keep this dependency at all, since it's only used in rootcling? Perhaps we can instead replace it with optparse.h (not because one is specifically better than the other but because it makes no sense keeping both and having one fewer dependency is desirable)

Edit: I guess the advantage of this is that it handles subcommands (which rootcling needs), so perhaps the better path would be to remove optparse in favor of this? Or maybe the current situation is fine, as long as both libraries do their job.

Edit2: Ah, but subcommands are not actually handled directly by OptionParser.h, it's done via llvm::SubCommand...

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

Test Results

    23 files      23 suites   3d 13h 29m 28s ⏱️
 3 856 tests  3 855 ✅ 0 💤 1 ❌
78 544 runs  78 542 ✅ 1 💤 1 ❌

For more details on these failures, see this check.

Results for commit f7d3cf3.

@ferdymercury

Copy link
Copy Markdown
Collaborator Author

Sure, I would vote for dropping this if CLI11 or optparse.h can be used instead

@silverweed silverweed left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For now this lgtm, to be discussed whether we want to get rid of it in the future or not

@silverweed silverweed self-assigned this Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip code analysis Skip the code analysis CI steps for this PR, including verifying clang-formatting and running Ruff.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants