Page MenuHomePhabricator

[LLDB] libcxx summary formatters for std::string_view
AcceptedPublic

Authored by puremourning on Oct 21 2021, 7:54 AM.

Details

Reviewers
jingham
shafik
Summary

When printing a std::string_view, print the referenced string as the
summary. Support string_view, u32string_view, u16string_view and
wstring_view, as we do for std::string and friends.

This is based on the existing fomratter for std::string, and just
extracts the data and length members, pushing them through the existing
string formatter.

In testing this, a "FIXME" was corrected for printing of non-ASCII empty
values. Previously, the "u", 'U" etc. prefixes were not printed for
basic_string<> types that were not char. This is trivial to resolve by
printing the prefix before the "".

Diff Detail

Event Timeline

puremourning requested review of this revision.Oct 21 2021, 7:54 AM
puremourning created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2021, 7:54 AM

I'm happy to revert/split out the change for empty strings, as this is perhaps contentious (and not exactly minimal for this patch).

Also, I'm only implementing libcxx (for now). We can follow up with libstdc++ in another patch if this is accepted.

This is my first lldb patch, so apologies if I did something out of ordinary.

I'm happy to revert/split out the change for empty strings, as this is perhaps contentious (and not exactly minimal for this patch).

I would actually prefer if we could split this into a separate change :) (Sorry!)

Also, I'm only implementing libcxx (for now). We can follow up with libstdc++ in another patch if this is accepted.

Sounds good to me.

This is my first lldb patch, so apologies if I did something out of ordinary.

Nope, I think this is actually looking pretty good. I just have a few nits that I'll try get around today/tomorrow.

I would actually prefer if we could split this into a separate change :) (Sorry!)

No problem, that was my gut feeling at the time too.

Remove change to the formatting of empty u16 and u32 strings

RE-post with clang-format in path

jingham added inline comments.Oct 21 2021, 10:07 AM
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string_view/TestDataFormatterLibcxxStringView.py
63

We have lldbtest.expect_var_path for doing this sort of test. It makes these tests easier to read and less likely to be fooled by accidental matches.

Use expect_var_path rather than just expect in test

Thanks for the review!

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string_view/TestDataFormatterLibcxxStringView.py
63

Done. There's s slight difference in this API - the null pointer summary returns "" via this api rather than the output which returns "nullptr", so I've inlaced both in the test.

shafik added a subscriber: shafik.Oct 21 2021, 11:52 AM

It looks good to me but can we verify it behaves nicely with undefined behavior cases e.g.:

std::string s = "Hellooooooooooooooo ";
std::string_view sv = s + "World\n";

and a case like the one that was fixed in D108228

I am not expecting any surprises since you are using the underlying string formatter but worth checking.

Revert some formatting lost in rebase

It looks good to me but can we verify it behaves nicely with undefined behavior cases e.g.:

std::string s = "Hellooooooooooooooo ";
std::string_view sv = s + "World\n";

Will add some cases, sure.

and a case like the one that was fixed in D108228

Thanks for the pointer! (no pun intended). I will apply the same change/mitigattion to my new code.

I am not expecting any surprises since you are using the underlying string formatter but worth checking.

yes, certainly is, thanks!

Add some tests for dodgy inputs.
Handle invalid cases more consistently.
Factor out some copy paste code.

puremourning marked an inline comment as done.Oct 21 2021, 2:01 PM

Sorry, but is anything further required from me on this patch ?

Sorry, but is anything further required from me on this patch ?

You addressed an issue Shafik asked you to, so then he should make sure he's happy with the change and mark the patch accepted. OTOH we're all busy so it's easy to let this sort of thing drop. On your end, you should wait a polite interval then issue a gentle ping in his direction... A week is an okay interval, I think, so now would be appropriate.

Sorry, but is anything further required from me on this patch ?

You addressed an issue Shafik asked you to, so then he should make sure he's happy with the change and mark the patch accepted. OTOH we're all busy so it's easy to let this sort of thing drop. On your end, you should wait a polite interval then issue a gentle ping in his direction... A week is an okay interval, I think, so now would be appropriate.

Sure, I wasn't nagging, just checking :) I completely understand how these things go. There's no rush.

shafik accepted this revision.Fri, Nov 19, 2:38 PM

Apologies, this dropped off my radar it LGTM, thank you!

This revision is now accepted and ready to land.Fri, Nov 19, 2:38 PM

Apologies, this dropped off my radar it LGTM, thank you!

Thanks!

Sorry for my ignorance, but what's the next step to land this? Do you want me to rebase it first and push that, or is someone abele land this for me, as I don't have commit rights ?