Fix for compiler error while indexing views: changed compile time check to runtime check. Fixes #2395

[goto40]

Checklist

• [X] The title and commit message(s) are descriptive.
• [X] Small commits made to fix your PR have been squashed to avoid history pollution. squash on merge: Commit text = changed compile time error to runtime error to prevent unintentional compiler error.
• [X] Tests have been added for new features or bug fixes.
• [X] API of new functions and classes are documented. n/a - no new functions added.
• [x] remove comments added for the review process

Description

Discussion of #2395.

A unittest is provided to demonstrate the problem (part of test_xview: regression_2395_access_compiler_problem).

Summary of the change:

• no runtime performance impact is expected
• The top level logic of the change is that in cases where an illegal call is present in the program a "throw" is generated instead of producing a compile time error.

[tdegeus]

Thanks for this PR. I'm still quite confused though. Could you try to explain, in words, the source of the problem? Also it seems that all you did was to replace a static assertion by a dynamic assertion. But is the static assertion fires, the dynamic will as well no?

[goto40]

But is the static assertion fires, the dynamic will as well no?

No. In the described cases (unit test), the problematic code is never called (protected by an if), but the body of the if produces code, which in turn fails with a static_assert. This was changed.

[tdegeus]

Could you explain (with some snippets) here in words what if is causing the issue?

[goto40]

in xview.hpp https://github.com/xtensor-stack/xtensor/blob/f3c11b2d810159e7063daddeaa0764f4006e5a73/include/xtensor/xview.hpp#L1572

    template <class CT, class... S>
    template <class Arg, class... Args>
    inline auto xview<CT, S...>::access(Arg arg, Args... args) -> reference
    {
        if (sizeof...(Args) >= this->dimension())        // <----- discard arg in some cases
        {
            return access(args...);
        }
        return access_impl(make_index_sequence(arg, args...), arg, args...);
    }

The code inside the if leads to the problem.

[goto40]

See also my comments above in this PR. There I try to explain my observations.

[tdegeus]

Thanks @goto40 . So it the first or the second case that leads to the static assertion?

So I ask, because all if this does work when you did

auto a0 = xt::view(a3d, 0, xt::all(), xt::all());
auto a1 = xt::view(a3d, xt::all(), 1, xt::all());

That is why I'm having troubles understanding what is different for

auto a2 = xt::view(a3d, xt::all(), xt::all(), 2);

In that case dimension() should still be 2 right? I'm not sure also why a dynamic if should trigger the static_assert. I'd rather think that there is an issue with some static dispatch. My intuition is that there might be an issue with (the use of) newaxis_count.

The problem is that I still don't grasp the source of the bug, so I'm having a hard time judging the workaround. That why I keep asking the same question ;)

[goto40]

Good point. I will also need more time to analyze... Please comment, as soon you have new insights!

[goto40]

Note: have you seen the comments with label1 and label2? Do you have an explanation why we have this difference: why we take integral_count and newaxis_count in account for one case, but not for the other?