Skip to content

fix(cpp1): support empty and multi-argument indexing #483

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

Merged
merged 2 commits into from
Jul 23, 2023

Conversation

JohelEGP
Copy link
Contributor

@JohelEGP JohelEGP commented Jun 1, 2023

Resolves #482.
Partially addresses #387 (operator[]).

The added test requires C++23.

Testing summary.
100% tests passed, 0 tests failed out of 684

Total Test time (real) =  98.23 sec
Acknowledgements.

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Jun 1, 2023

As you can see, this needs to run in C++23 only.

I'll have to enhance https://github.com/modern-cmake/cppfront for this.
Someone will have to do the same for the scripts bundled in this repository.

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Jun 1, 2023

See the generated test passing at https://cpp2.godbolt.org/z/PfoWWqznG in C++23.

@MaxSagebaum
Copy link
Contributor

One question would be how the range checker can be extended to multiple dimensions. I do not know the standard proposal for this therefore I ask the question: How does the c++ standard defines the size operator for multi dimensional arrays? Can we use this mechanism to check the range of other dimensions?

@@ -3065,6 +3066,7 @@ class cppfront
if (
flag_safe_subscripts
&& i->op->type() == lexeme::LeftBracket
&& std::ssize(i->expr_list->expressions) == 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a TODO, that this needs to be extended for multiple dimensions.

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 think that perhaps the correct thing to do
is to only avoid cpp2::assert_in_bounds when there are no index arguments,
and add a multi-dimensional overload for cpp2::assert_in_bounds that doesn't assert.
Then someone versed in std::mdspan can just add an overload for it that asserts.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks! I think it's fine to do only single-dimensional bounds checks for the time being -- it will be quite a while before multidimensional is actually usable/used in the field.

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Jun 2, 2023

How does the c++ standard defines the size operator for multi dimensional arrays?

There's none.
The resolved #482 links to the only occurrence in the C++ standard library:
https://en.cppreference.com/w/cpp/container/mdspan/operator_at.

@MaxSagebaum
Copy link
Contributor

Ok thanks for applying my remarks.

It doesn't seem trivial to support multi-argument indexing in cpp2::assert_in_bounds.
See https://en.cppreference.com/w/cpp/container/mdspan/operator_at.

I had a look at cpp2::assert_in_bounds. Out of my had, one option would be:

template<typename ... Args>
auto assert_in_bounds_multi(auto&& x CPP2_SOURCE_LOCATION_PARAM_WITH_DEFAULT, Args&& ... args ) -> decltype(auto)
    requires ((std::is_integral_v<CPP2_TYPEOF(args)> && ...) &&
             requires { std::ssize(x, 1); x[arg]; std::begin(x) + 2; })
{
   
    int dim = 0;
    (Bounds.expects(0 <= args && args < std::ssize(x, dim++), "out of bounds access attempt detected" CPP2_SOURCE_LOCATION_ARG)), ...;
    return CPP2_FORWARD(x) [ CPP2_FORWARD(args)... ];
}

I have not tested this. The increment of dim in the expansion is kind of hacky. Here we would assume that there is a std::ssize(container, dim) function. Would this make sense?

Do you want to pursue such an implementation or should we leave it. I you want I can try and implement it.

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Jun 2, 2023

That doesn't work for std::mdspan.
See how its precondition has to look like: https://eel.is/c++draft/containers#mdspan.mdspan.members-note-1.

It could work for a container of containers
that added support for multi-dimensional indexing.

Without actual use-cases, it's hard to argue in favor of an overload that asserts.

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Jun 2, 2023

[Note 1: This implies that map_(I) < map_.required_span_size() is true.
— end note]

It seems like std::mdspan provides the interface to implement that precondition,
but I seems to be a pack.
Also, it can't be tested today.
Here's the implementation status, with libc++ noted "std::extents only".
1685717383
1685717394

{
error("subscript expression [ ] must not be empty (if you were trying to name a C-style array type, use 'std::array' instead)");
next();
Copy link
Owner

@hsutter hsutter Jul 23, 2023

Choose a reason for hiding this comment

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

We should still have an error here though, should we? Like the one for ( at line 4181 below.

Signed-off-by: Herb Sutter <herb.sutter@gmail.com>
@hsutter hsutter merged commit 84ec819 into hsutter:main Jul 23, 2023
@JohelEGP JohelEGP deleted the empty_index branch July 23, 2023 19:27
zaucy pushed a commit to zaucy/cppfront that referenced this pull request Dec 5, 2023
* fix(cpp1): support empty and multi-argument indexing

* Make error message for `[` arg case symmetric with `(` arg case

Signed-off-by: Herb Sutter <herb.sutter@gmail.com>

---------

Signed-off-by: Herb Sutter <herb.sutter@gmail.com>
Co-authored-by: Herb Sutter <herb.sutter@gmail.com>
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.

[BUG] No support for multi-argument indexing
3 participants