Skip to content

Conversation

@pps83
Copy link
Contributor

@pps83 pps83 commented Dec 9, 2024

std::string_view should be shown similar to std::string in debugger for basic view.

@pps83 pps83 requested a review from a team as a code owner December 9, 2024 17:41
@pps83
Copy link
Contributor Author

pps83 commented Dec 9, 2024

here's relevant bug report https://developercommunity.visualstudio.com/t/Bad-debug-visualizers-for-std::string_vi/10808901

in short, here's how it shows in debugger:
image

here's how it should show (after this fix):
image

@StephanTLavavej StephanTLavavej added the visualizer How the VS debugger displays STL types label Dec 9, 2024
@CaseyCarter
Copy link
Contributor

I pushed a small change to add the same na format specifier to [string_view] in the expanded view of _String_view_iterator.

@pps83
Copy link
Contributor Author

pps83 commented Dec 10, 2024

What do you guys @StephanTLavavej @CaseyCarter think about my last comment, specifically this part:

This also means that perhaps std::basic_string should be rewritten without using these overloads for su and s32.

update: basic_string works for char8_t because it has

<AlternativeType Name="std::basic_string&lt;char8_t,*&gt;" />

perhaps, this all can be simplified with single version that uses na

@pps83
Copy link
Contributor Author

pps83 commented Dec 10, 2024

an unrelated debugger issue, maybe you can pass it on internally within VS team:
(things like wchat_t{} aren't handled properly, while wchar_t() is ok)

image

@CaseyCarter
Copy link
Contributor

What do you guys @StephanTLavavej @CaseyCarter think about my last comment, specifically this part:

This also means that perhaps std::basic_string should be rewritten without using these overloads for su and s32.

I'd be happy to see this in a separate PR.

@pps83
Copy link
Contributor Author

pps83 commented Dec 10, 2024

I'd be happy to see this in a separate PR.

Yes, that's irrelevant for this PR. Here's simplified visualizers for std::string
Not sure if that should be merged though, as it doesn't fix anything, but perhaps could break some versions of VS.

@pps83
Copy link
Contributor Author

pps83 commented Dec 10, 2024

FYI, this PR works with all char types except with unsigned short. To make it work, separate overload for <Type Name="std::basic_string_view&lt;unsigned short,*&gt;"> needs to be added. Note, this is actually needed only when wchar_t is native! If it's not native (eg /Zc:wchar_t- is set in project options), then unsigned short works without any issues without extra overload. There is however, an issue with string_view::iterator for unsigned short: it uses traits as a template param, so not clear what's the best way to provide overload for the case (should perhaps use std::type_traits<unsigned short> there)

One other issue: string_view::iterator doesn't look like string::iterator in debugger. This also needs to be addressed.

Should this all be done in this PR?

    std::string s{"cute fluffy kittens \U0001F408"};
    std::wstring_view wsv{L"long long cats \U0001F408"};

    std::string::iterator si = s.begin() + 1;
    std::wstring_view::iterator zzi = wsv.begin() + 1;

image

@CaseyCarter
Copy link
Contributor

CaseyCarter commented Dec 10, 2024

FYI, this PR works with all char types except with unsigned short. To make it work, separate overload for <Type Name="std::basic_string_view&lt;unsigned short,*&gt;"> needs to be added. Note, this is actually needed only when wchar_t is native! If it's not native (eg /Zc:wchar_t- is set in project options), then unsigned short works without any issues without extra overload. There is however, an issue with string_view::iterator for unsigned short: it uses traits as a template param, so not clear what's the best way to provide overload for the case (should perhaps use std::type_traits<unsigned short> there)

We only care that basic_string<unsigned short> and basic_string_view<unsigned short> and their iterators visualize correctly with /Zc:wchar_t-. With /Zc:wchar_t-, they should like a lot like basic_string<wchar_t> et al. do with /Zc:wchar_t. Since this PR only adds some address suppression via na format specifiers, I don't think it should have regressed the visualization of any meow<unsigned short> types.

One other issue: string_view::iterator doesn't look like string::iterator in debugger. This also needs to be addressed.

Should this all be done in this PR?

    std::string s{"cute fluffy kittens \U0001F408"};
    std::wstring_view wsv{L"long long cats \U0001F408"};

    std::string::iterator si = s.begin() + 1;
    std::wstring_view::iterator zzi = wsv.begin() + 1;

image

I see, it looks like we're missing some ,na in the SmartPointer elements of the _String_view_iterator visualizers. That change is consistent with the intent of this PR, I think it should happen here.

@pps83
Copy link
Contributor Author

pps83 commented Dec 10, 2024

I didn't want to drastically change the PR, but this is the version that works best for me: STL.natvis: Fix visualization for string_view

Let me know is I should override this PR with that diff.

In short, it works for all types and matches DisplayString or StringView for string_view and string_view::iterator.

I used this code to test. Note, the test code has weird stuff like const short and others: I simply wanted to see how it would look like, but didn't try to make it work with those types.

template<class T>
void testIter(T x)
{
    auto s = x.substr(1, 5);
    std::basic_string<std::remove_cv_t<typename T::value_type>> ss(s.data(), s.size());
    typename T::iterator it = s.begin() + 1;
    typename T::const_iterator itc = s.begin() + 1;

    typename std::basic_string<std::remove_cv_t<typename T::value_type>>::iterator sit = ss.begin() + 1;
    typename std::basic_string<std::remove_cv_t<typename T::value_type>>::const_iterator sitc = ss.begin() + 1;

    s.empty();
}

void build_schema()
{
    std::string s{"cute fluffy kittens \U0001F408"};

    std::string_view sv{"long long cats \U0001F408"};
    std::basic_string_view<const char> sv0{"long long cats \U0001F408"};
    std::u8string_view svc{u8"long long cats \U0001F408"};
    std::wstring_view wsv{L"long long cats \U0001F408"};
    std::u16string_view wsvc{u"long long cats \U0001F408"};
    std::basic_string_view<unsigned short> wsvu = {(unsigned short*)wsv.data(), wsv.size()};
    std::basic_string_view<const unsigned short> wsvu1 = {(unsigned short*)wsv.data(), wsv.size()};
    std::basic_string_view<const short> wsvsu = {(short*)wsv.data(), wsv.size()};
    std::basic_string_view<short> wsvsu1 = {(short*)wsv.data(), wsv.size()};
    std::u32string_view u32sv{U"long long cats \U0001F408"};

    testIter(sv);
    testIter(sv0);
    testIter(svc);
    testIter(wsv);
    testIter(wsvc);
    testIter(wsvu);
    testIter(wsvu1);
    testIter(wsvsu);
    testIter(wsvsu1);
    testIter(u32sv);
}

image

It has only minor inconsistency with std::string for char16_t where debugger shows u"ng L" vs L"ng L" for string_view due to su cast.

image

@CaseyCarter
Copy link
Contributor

sizeof(meow) == 2 seems problematic; programs can define custom character types of size 2 that have nothing to do with UTF-16. Why reintroduce su formatting if it's unnecessary for basic_string<wchar_t> and basic_string_view<wchar_t> with /Zc:wchar_t-?.

@pps83
Copy link
Contributor Author

pps83 commented Dec 10, 2024

sizeof(meow) == 2 seems problematic; programs can define custom character types of size 2

Yes, that's problematic. As mentioned in the other thread, perhaps it all can be eliminated

`std::string_view` should be shown similar to `std::string` in debugger for basic view.
@pps83 pps83 force-pushed the main-stringview-visualizer branch from b29ee9a to 432abba Compare December 10, 2024 19:37
@pps83
Copy link
Contributor Author

pps83 commented Dec 10, 2024

I used this code to test:

template<class T>
void testIter(T x)
{
    auto s = x.substr(2, 10);
    std::basic_string<std::remove_cv_t<typename T::value_type>> ss(s.data(), s.size());
    typename T::iterator it = s.begin() + 1;
    typename T::const_iterator itc = s.begin() + 1;

    typename std::basic_string<std::remove_cv_t<typename T::value_type>>::iterator sit = ss.begin() + 1;
    typename std::basic_string<std::remove_cv_t<typename T::value_type>>::const_iterator sitc = ss.begin() + 1;

    s.empty();
}

void build_schema(const char* slnFile)
{
    std::string s{"cute fluffy kittens \U0001F408"};

    std::string_view sv{"a long \U0001F408 long cats"};
    std::u8string_view sv8{u8"a long \U0001F408 long cats"};
    std::wstring_view svw{L"a long \U0001F408 long cats"};
    std::u16string_view sv16{u"a long \U0001F408 long cats"};
    std::u32string_view sv32{U"a long \U0001F408 long cats"};

    testIter(sv);
    testIter(sv8);
    testIter(svw);
    testIter(sv16);
    testIter(sv32);
}

All visualizers for string/string_view (using char, char8_t, char16_t, char32_t, wchar_t) and their iterators now look consistent. With /Zc:wchar_t- or without it. With _ITERATOR_DEBUG_LEVEL=0 or without it. (minor diff being that _ITERATOR_DEBUG_LEVEL=0 eliminates size from string_view::iterator).

This is sample debugger view for wchar_t with /Zc:wchar_t- (as you can see it shows up as unsigned short overload on the right.

image

@StephanTLavavej StephanTLavavej self-assigned this Dec 12, 2024
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 56bd1fe into microsoft:main Dec 13, 2024
39 checks passed
@StephanTLavavej
Copy link
Member

Thanks for making this simpler, and congratulations on your first microsoft/STL commit! 💚 🎉 😸

We still need to "triple-mirror" this change into VS before it will actually take effect.

@pps83 pps83 deleted the main-stringview-visualizer branch December 13, 2024 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

visualizer How the VS debugger displays STL types

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants