Skip to content

[PROPOSE] Refactor args_t to use ranges instead of vector #380

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

Closed

Conversation

filipsajdak
Copy link
Contributor

In the current cppfront implementation (65fcd0f), we are using std::vector<std::string_view> to store args in main(). This change proposes to use std::ranges that can use lazy evaluation of ranges.

Thanks to this change we can avoid one allocation that is unnecessary.

Copy link
Contributor

@JohelEGP JohelEGP left a comment

Choose a reason for hiding this comment

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

Definitely an improvement.

You could try either of these to not make args_t a template: https://compiler-explorer.com/z/E9r7xcoMo.

@filipsajdak
Copy link
Contributor Author

I have been thinking about your approach - I did not like the lambda living somewhere outside. If Herb will be open to making it dependent on ranges we can choose one solution that will fit the best.

Thanks for feedback!

@JohelEGP
Copy link
Contributor

JohelEGP commented Apr 17, 2023

Only its type lives outside. The view holds its instance.

@filipsajdak filipsajdak force-pushed the fsajdak-args-with-ranges branch 2 times, most recently from 8b3d1a3 to 72799b1 Compare April 18, 2023 06:30
@filipsajdak
Copy link
Contributor Author

@JohelEGP I have slept over it and decided to follow the path you propose but move the lambda directly to the type definition - like here: https://godbolt.org/z/Mvv6a4rs6 (I think the code looks simpler).

Thanks for the excellent feedback!

Copy link
Contributor

@JohelEGP JohelEGP left a comment

Choose a reason for hiding this comment

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

Yeah, that's much better. Thank you.

@filipsajdak filipsajdak force-pushed the fsajdak-args-with-ranges branch from 72799b1 to 0e95e4b Compare April 19, 2023 16:37
@hsutter
Copy link
Owner

hsutter commented Apr 19, 2023

Thanks! I thought about using ranges. Because Ranges is not yet widely supported I'm reluctant to introduce a dependency on it without a strong justification. (Note that cppfront should support ranges -- they should work fine if you're using a Cpp1 compiler that supports them. But it doesn't require ranges including by injecting a ranges dependency into the user's program, that's what I'm getting at. If we did this PR, then a user program that didn't use ranges at all would not work on a Cpp1 compiler that is otherwise fine but doesn't yet support ranges.)

The cppreference C++20 conformance chart shows Ranges as only "partial" in upstream Clang, until version 15 which was released last month(? and still has an asterisk), and as you mentioned in this comment on #262 that links this PR:

my apple-clang-14 has just provided support for ranges

So I'm inclined to reject this PR on the grounds that Ranges is not yet supported widely/robustly enough (particularly in Clang/libc++) to be safe to take a dependency on without having a compelling reason. A compelling reason would be something like that a major feature of cppfront doesn't work or is so slow that it inhibits adoption. Saving one allocation doesn't meet that bar (sorry!). But please reopen this if you think I'm making a mistake.

Thanks, and thanks for understanding!

@hsutter hsutter closed this Apr 19, 2023
@filipsajdak
Copy link
Contributor Author

That is fine. The implementation is ready when there will be time to introduce it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants