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

Unicode API rework #923

Merged
merged 9 commits into from
Sep 20, 2023
Merged

Unicode API rework #923

merged 9 commits into from
Sep 20, 2023

Conversation

bindreams
Copy link
Contributor

@bindreams bindreams commented Sep 18, 2023

Fixes #845 as discussed.

Comparing the two approaches of getting argv:

  1. The "old" way, through CLI::argv():
    ✔️ Works automatically and almost everywhere
    ❌ Small abstraction overhead on macOS
    ❌ Does not work in weird edge-cases such as missing /proc
  2. This PR, through app.ensure_utf8:
    ✔️ True zero-overhead abstraction: you don't pay for what you don't use
    ✔️ Less moving parts than the "old" approach, probably can't be broken
    ❌ Requires extra code to be written by the user (which is sad because ideally everyone should use this by default)

@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed Coverage
include/CLI/App.hpp ø
include/CLI/impl/Argv_inl.hpp ø
include/CLI/impl/App_inl.hpp 100.00%

📢 Thoughts on this report? Let us know!.

@bindreams
Copy link
Contributor Author

bindreams commented Sep 18, 2023

@henryiii PTAL. Implemented this before your comment about axing CLI::argv. The way it works here is CLI::argv is available but completely optional. Do I still cut it out? I'll be honest, I'm sad to see it go 😢.

@bindreams bindreams marked this pull request as ready for review September 18, 2023 20:55
@henryiii
Copy link
Collaborator

I was thinking this:

int main(int argc, char** argv) {
     CLI::App app{"App description"};
     argv = app.ensure_utf8(argc, argv);
     CLI11_PARSE(app);
     return 0;
}

That is, calling ensure_utf8 stores the (new on windows) argv in the app on all platforms. Then it's legal to call CLI11_PARSE with just app, no additional magic needed. That's a little bit more complicated than before, but slightly better than having to list argc/argv again. You don't even need the argv = part if you aren't going to use it later. If it hasn't been stored, I was originally thinking we could make CLI11_PARSE(app) an error, though if you still wanted to have the CLI::argv() work, then it could use that.

The downside is it's not as obvious how inject modification of argv in this form, of course.

These are the possible usages:

// 1
app.ensure_utf8(argc, argv);
CLI11_PARSE(app);  // Uses stored argv vector on all platforms

// 2
argv = app.ensure_utf8(argc, argv);
CLI11_PARSE(app, argc, argv);  // Uses produced argv vector

// 3
CLI11_PARSE(app, argc, argv); // Not unicode

// 4
CLI11_PARSE(app); // Not portable, but unicode

My favorite probably is 2, as it's explicit, but 1 is pretty simple looking for most users who don't manipulate argv first, and can be written as 2 if a user does need to. I'm fine to drop 4 as too magical and not portable, but if we keep CLI::argv(), we could keep that too. Some people might like the brevity and not care about portability. But 1 is pretty simple.

@henryiii
Copy link
Collaborator

henryiii commented Sep 19, 2023

This looks like an improvement, my main question is, would it make sense to support (1) above and maybe drop (4)? (In later PRs)

@henryiii
Copy link
Collaborator

@phlptp thoughts about API in follow-ups? I’ll approve this PR tomorrow.

@henryiii henryiii merged commit 0d4e191 into CLIUtils:main Sep 20, 2023
50 checks passed
@github-actions github-actions bot added the needs changelog Hasn't been added to the changelog yet label Sep 20, 2023
@phlptp
Copy link
Collaborator

phlptp commented Sep 20, 2023

I don't have any strong opinions on the unicode stuff, so this seems reasonable to me.

@bindreams
Copy link
Contributor Author

Thanks for the merge!

Would you mind putting in a word with the all-contributors bot about my contributions? Sorry for being indiscreet.

@henryiii henryiii mentioned this pull request Jan 11, 2024
@henryiii
Copy link
Collaborator

@all-contributors, please add @andreasxp for code

@henryiii
Copy link
Collaborator

(Not totally sure the bot is still working, we might need to update manually, feel free to ping (a lot) if we forget!)

@henryiii
Copy link
Collaborator

Done in #977.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs changelog Hasn't been added to the changelog yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI::argv() crashes on linux when /proc is missing
3 participants