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

<mdspan>: Using std::mdspan results in multiple warnings (C5246) #4477

Closed
dasmysh opened this issue Mar 13, 2024 · 5 comments · Fixed by #4527
Closed

<mdspan>: Using std::mdspan results in multiple warnings (C5246) #4477

dasmysh opened this issue Mar 13, 2024 · 5 comments · Fixed by #4527
Labels
bug Something isn't working fixed Something works now, yay!

Comments

@dasmysh
Copy link

dasmysh commented Mar 13, 2024

Describe the bug

When just initializing an std::mdspan with dynamic extents multiple warnings (C5246) are produced during compilation.

Command-line test case

C:\Temp>type mdspan_testcase.cpp
#include <array>
#include <mdspan>

int main()
{
    std::array test_data{0, 1, 2, 3, 4, 5};
    std::mdspan<int, std::dextents<std::size_t, 2>> test_span(test_data.data(), 2, 3); // mdspan(329,54): warning C5246 + mdspan(124,50): warning C5246
    return 0;
}

C:\Temp>cl /EHsc /Wall /WX /std:c++latest .\mdspan_testcase.cpp
[...]

multiple C5246 warnings occur.

Expected behavior

Compiles without warnings.

STL version

```
Microsoft Visual Studio Community 2022 (64-Bit) - Preview
Version 17.10.0 Preview 1.0
```
@StephanTLavavej
Copy link
Member

In general, the STL doesn't attempt to be /Wall clean, only /W4 (and /analyze). We sometimes fix off-by-default warnings on a case-by-case basis though, especially when they appear to be more valuable than noisy.

In this case, I can't immediately see what code is triggering these warnings; the line numbers don't make sense to me.

@frederick-vs-ja
Copy link
Contributor

The warning actually comes form here.

STL/stl/inc/mdspan

Lines 121 to 124 in d6efe94

template <class _ExtsTuple, size_t... _Indices>
requires (tuple_size_v<_ExtsTuple> == _Rank_dynamic)
constexpr explicit extents(_Extents_from_tuple, _ExtsTuple _Tpl, index_sequence<_Indices...>) noexcept
: _Base{static_cast<index_type>(_STD move(_STD get<_Indices>(_Tpl)))...} {}

The implementation currently depends on brace elision in aggregate initialization, but I think this should be OK. IMO warning C5246 doesn't make much sense.

@dasmysh
Copy link
Author

dasmysh commented Mar 14, 2024

IMO warning C5246 doesn't make much sense.

So you would say this is a compiler issue? I had a feeling it would be, but since the std::mdspan implementation is rather new I wanted to check here first. I would raise it there then.

@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Mar 14, 2024

So you would say this is a compiler issue?

IMHO warning on "the initialization of a subobject should be wrapped in braces" is itself a bad design. Such warning discourages initializing std::array in a standard-guaranteed way ([array.overview]/2).

At least I think we should suppress the warning in <mdspan>.

@StephanTLavavej
Copy link
Member

We talked about this at the weekly maintainer meeting. While the STL doesn't attempt to be /Wall clean in general, we think it would be reasonable to add an STL-wide suppression for C5246. Our rationale is that we can't remember ever having incorrect code in the STL caused by brace elision, so the warning seems exceptionally low-value for us.

The place to do so is around here:

// warning C4514: unreferenced inline function has been removed (/Wall)

Note that we capture the exact warning text, followed by "(/Wall)" for off-by-default warnings.

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Mar 20, 2024
@StephanTLavavej StephanTLavavej added bug Something isn't working and removed enhancement Something can be improved labels Mar 26, 2024
@StephanTLavavej StephanTLavavej added the fixed Something works now, yay! label Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed Something works now, yay!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants