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

Implement std::size and std::ssize for C-style arrays #57

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

davidtranhq
Copy link
Contributor

C++17 and C++20 defines template functions for deducing the size of C-style arrays. I've found this useful and the implementation is simple (pretty much identical to the implementation on cppreference).

@64
Copy link
Member

64 commented Dec 30, 2022

LGTM, anyone else have objections to merging this? Alternatively it could live in cxxshim?

Copy link
Member

@Dennisbonke Dennisbonke left a comment

Choose a reason for hiding this comment

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

Two style nitpicks, other than that this looks good to me.

include/frg/array.hpp Outdated Show resolved Hide resolved
include/frg/array.hpp Outdated Show resolved Hide resolved
Copy link
Member

@no92 no92 left a comment

Choose a reason for hiding this comment

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

I'd prefer the second commit to be squashed into the first, other than that LGTM.

@ArsenArsen
Copy link
Member

Don't hijack std:: in frigg

@64
Copy link
Member

64 commented Jan 1, 2023

Don't hijack std:: in frigg

i.e this could be changed to use the frg namespace or you could send a PR to cxxshim and put it there instead.

@davidtranhq
Copy link
Contributor Author

Don't hijack std:: in frigg

yeah, not sure what's preferred here. I wanted it to match the corresponding get functions that were also overloaded in the std namespace + thought it fit better in frigg since the standard library defines these functions in <array>. Up to the maintainers where they want it to go.

@ArsenArsen
Copy link
Member

<array> is included in freestanding in GCC 13. Either use GCC 13 snapshots (and more testing is always nice :) or a hack (though, you'd need to add it to this hack in particular).

Either way, frigg should not clash with the standard, and more conservatively, the standard namespace.

I recommend the GCC 13 route if you can afford using a compiler with potential regressions (and, again, finding and reporting such regressions is invaluable).

@ArsenArsen
Copy link
Member

oh, also, FTR: adding stuff to std:: is explicitly UB, unless it's to specialize existing templates like is the case for std::get and std::tuple_{size,element}, which is why frigg works in hosted environments without this overload added.

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.

5 participants