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

Fix debugger visualizer for Ext=gsl::dynamic_extent #857

Merged
merged 1 commit into from
May 21, 2020

Conversation

pps83
Copy link
Contributor

@pps83 pps83 commented Mar 27, 2020

VS 2019 doesn't seem to match -1 for size_t template parameter, as a result dynamic span/basic_string_span/basic_zstring_span show extent as extent = 4294967295 (for 32-bit builds). This change updates details::extent_type to have static constexpr size_ parameter for non-dynamic span/basic_string_span/basic_zstring_span and simplifies/removes dynamic versions from GSL.natvis

fixes #856

@msftclas
Copy link

msftclas commented Mar 27, 2020

CLA assistant check
All CLA requirements met.

VS 2019 doesn't seem to match -1 for size_t template parameter, as a result dynamic span/basic_string_span/basic_zstring_span show extent as `extent = 4294967295` (for 32-bit builds). This change updates details::extent_type to have static constexpr size_ parameter for non-dynamic span/basic_string_span/basic_zstring_span and simplifies/removes dynamic versions from GSL.natvis

fixes microsoft#856
Copy link
Contributor

@JordanMaples JordanMaples left a comment

Choose a reason for hiding this comment

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

The change looks good, I suggest wrapping the definition for size_ in a debug check since the template arg is used for all other operations.

@pps83
Copy link
Contributor Author

pps83 commented Apr 18, 2020

The change looks good, I suggest wrapping the definition for size_ in a debug check since the template arg is used for all other operations.

.natvis works in release as well, are you sure that wrapping it in ifdef _DEBUG will not break it? I'd rather update extent_type to use size_ instead

@JordanMaples
Copy link
Contributor

Maintainers' call: While you're correct that the natvis works for debugging code build with release, the natvis is ultimately a debugging tool. GCC and Clang users won't get any benefit from the additional size_ variable. We shouldn't force all consumers to pay for the ability to debug in release mode.

Please add the _DEBUG check and I'll merge it in. Thank!

@pps83
Copy link
Contributor Author

pps83 commented May 14, 2020

Maintainers' call: While you're correct that the natvis works for debugging code build with release, the natvis is ultimately a debugging tool. GCC and Clang users won't get any benefit from the additional size_ variable. We shouldn't force all consumers to pay for the ability to debug in release mode.

Please add the _DEBUG check and I'll merge it in. Thank!

Can you please explain what kind of cost this change would introduce? It's a static variable, not much different than using template's Ext param directly. I'd even suggest doing it this way instead:
pps83@bacb9c5#diff-912da93c0c34962d231e98d0a2be2dc7

@CaseyCarter
Copy link
Member

It's not only static, but implicitly inline so the variable definition won't be emitted into OBJs for translation units that don't use it - it's as close to free as you can get (https://godbolt.org/z/gUYExD). MSVC treats such inline constants specially and emits them into PDBs even when the program doesn't ODR-use them so they're accessible to the debugger for NATVIS purposes. (Most of the STL.natvis visualizers use enumerator constants for historical reasons, but more recent visualizers use static constexpr members like this.)

@pps83
Copy link
Contributor Author

pps83 commented May 16, 2020

It's not only static, but implicitly inline so the variable definition won't be emitted into OBJs for translation units that don't use it - it's as close to free as you can get (https://godbolt.org/z/gUYExD).

I wanted to mention that inline consts would be even better, but GSL is C++14. I just realized that they are implicitly inline :)
@JordanMaples what do you think, should I update to PR to using static const (or enum)?

@JordanMaples JordanMaples self-requested a review May 18, 2020 17:54
@JordanMaples
Copy link
Contributor

@pps83 let's use the static const.

@CaseyCarter thanks for jumping in and clearing that up.

@JordanMaples JordanMaples merged commit 283e314 into microsoft:master May 21, 2020
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.

[natvis] gsl::span<T, gsl::dynamic_extent> shows { extent = 4294967295 } in debugger
4 participants