You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Note, it does break std::basic_string<unsigned short> case (std::basic_string<short> didn't work before or after). With the change applied unsigned short and short versions look consistent at least.
Thanks! π» I pushed a comment update and two simplifications for wildcards.
β οΈI'm having trouble getting the VS IDE to pick up the updated natvis (I couldn't get it to display an added "taxicab" node), so I was unable to verify these changes, and I'm totally YOLOing them. Please double-check @CaseyCarter and @pps83 - I apologize if I messed up the PR.
For future reference, after code review begins, please avoid force-pushing changes as that makes it harder to incrementally review what changed.
Update: On Discord, Casey figured out why adding stl.natvis to my project didn't appear to be taking effect:
It looks like the debugger is preferring e.g. the more-specific std::basic_string<char,*> from the installed STL.natvis to our std::basic_string<*>. (Changing "Tools > Options > Debugging > Output Window > Natvis diagnostic messages" to "Verbose" is handy for debugging visualizer weirdness.) I do see other visualizers from the repo activating (Natvis: C:\STL\stl\debugger\STL.natvis(1041,6): Successfully parsed expression '_Mydata' in type context 'std::basic_string_view<char,std::char_traits<char> >'.) so it appears that https://learn.microsoft.com/en-us/visualstudio/debugger/create-custom-views-of-native-objects?view=vs-2022#BKMK_natvis_location is telling the truth about the search order for visualizers within natvis files.
It looks like we'll need to rename the installed STL.natvis with some other extension so it won't be loaded and we can cleanly test the incoming STL.natvis.
I didn't realize that all loaded visualizers were effectively being thrown into a giant pile, and the preference order for file fallbacks was being applied after "what visualizer is more specialized". I thought the file fallbacks were coarse-grained, but the actual behavior makes more sense, even if it's annoying for this development scenario.
This meant that we weren't actually testing anything, both before and after my changes (std::basic_string<*,*> was still less specific than the original std::basic_string<char,*>). Casey re-validated everything with no fallback so we're good now.
I also updated CPPDebuggerVisualizers, it appears boost::string didn't even have correct visualizers. What's interesting, it has InternalBufferCharsvariable to avoid calculations in string::capacity for short strings. Perhaps, ms/STL has something like that (or at least it should) and avoid doing this in visualizers:
IIRC there was an issue with one of the compilers (clang-cl?) not emitting static constexpr class members into the PDB when we last overhauled std::string so we replicated the calculation in the visualizer as a workaround. It may not be necessary anymore, but nobody has investigated.
Thanks for this dramatic simplification, and congratulations on your second microsoft/STL commit! π πββ¬ π»
idea for another commit: there is a paper about EAFs that allow faster date/time conversion math. It is like 20-30% faster than current algo in _Days_from_civil/_Civil_from_days
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It appears that debugger visualizer accepts
na
regardless if underlying char type