This is an archive of the discontinued LLVM Phabricator instance.

Add libc++ data formatter for std::variant
ClosedPublic

Authored by shafik on Aug 30 2018, 3:56 PM.

Diff Detail

Event Timeline

shafik created this revision.Aug 30 2018, 3:56 PM
vsk added a subscriber: vsk.Sep 10 2018, 4:56 PM
vsk added inline comments.
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/variant/TestDataFormatterLibcxxVariant.py
71

Could you add a test which inspects a reference to a variant (to cover the "(( )?&)?" bit you're matching)?

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/variant/main.cpp
30

Does a std::variant containing a std::variant work?

jingham requested changes to this revision.Sep 10 2018, 5:03 PM

A bunch of little comments and two more substantial bits.

  1. Can you add a test where we do "frame variable" on a one of these variants when it has one value, and then continue to a point where it has a different value and make sure we pick up the change. I think that's just a matter doing some "frame var"'s at your first breakpoint, and then again when you stop the second time. "frame var" maintains some state in the target (because the underlying objects need to support "value has changed") and it's always worthwhile to make sure that that doesn't interfere with picking up changes in value.
  1. See the embedded comment around line 100 in LibCxxVariant.cpp. You can't assume that "unsigned int" on the target and the lldb host are the same size.
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/variant/TestDataFormatterLibcxxVariant.py
30–42

Lines 29-41 can all be done with:

(self.target, self.process, _, bkpt) = lldbutils.run_to_source_breakpoint(self, "break here", SBFileSpec("main.c"))

source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp
28

I don't think you want the trailing "and" here.

66

obtain

85

to -> two?

93

Sadly this needs to be wrapped to 80 characters somewhere.

98–106

I don't think this comparison is a safe thing to do. For instance, you are comparing the unsigned value that you get from the target (index_value) with lldb's host "unsigned int" value. If the target has a 32 bit unsigned int, but the host has a 64 bit unsigned int (or vice versa) then the two values won't be the same.

274

Do you need the "formatv" here? Looks like you are just making a ConstString from "Value"? Maybe something got lost?

This revision now requires changes to proceed.Sep 10 2018, 5:03 PM
shafik updated this revision to Diff 165201.Sep 12 2018, 9:35 PM
shafik marked 9 inline comments as done.

Address comments on std::variant formatter

  • Adding tests for reference to variant and variant of variant
  • Refactoring test to be more efficient
  • Refactoring unsafe code that assumed sizes of types between local machine and target machine were the same
    • Used to check to variant_npos which is max unsigned or -1

@vsk @jingham I believe I have addressed your comments, please review again.

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/variant/main.cpp
30

Yes, adding test.

source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp
98–106

I believe I applied the change as we discussed earlier, let me know if not.

jingham accepted this revision.Sep 13 2018, 10:45 AM

Yes, that's a good way to do it.

This revision is now accepted and ready to land.Sep 13 2018, 10:45 AM
vsk added a comment.Sep 13 2018, 11:29 AM

Please clang-format your diffs.

@vsk Interesting I ran git clang-format before generating the diff and it made changes, so I am not sure what happened

shafik updated this revision to Diff 165536.Sep 14 2018, 10:44 AM

Applying clang-format

teemperor requested changes to this revision.Sep 14 2018, 10:51 AM
teemperor added a subscriber: teemperor.

LibCxx.h/.cpp are still not clang-formatted. I ran clang-format over the patch and pushed it here that this patch doesn't get stuck on this minor detail: https://github.com/Teemperor/lldb/commit/1c5c2f8f5d5af00420559dee52c9155e5bc72518.diff

This revision now requires changes to proceed.Sep 14 2018, 10:51 AM
shafik updated this revision to Diff 165540.Sep 14 2018, 10:59 AM

Applying clang-format to files I missed earlier

@teemperor Thanks for the catch!

teemperor accepted this revision.Sep 14 2018, 11:08 AM

LGTM now, thanks for the patch!

This revision is now accepted and ready to land.Sep 14 2018, 11:08 AM
This revision was automatically updated to reflect the committed changes.
shafik updated this revision to Diff 166144.Sep 19 2018, 10:26 AM

Updating LibcxxVariantGetIndexValidity() to no longer do type check of __index. It was left over from the old method of checking for an empty variant and was also breaking clang 5.