Adding data formatter for libc++ std::variant and appropriate tests
Details
- Reviewers
jingham davide EricWF teemperor - Commits
- rG8306f76e5632: [DataFormatters] Add formatter for C++17 std::variant
rG854a35092c25: [DataFormatters] Add formatter for C++17 std::variant
rLLDB342563: [DataFormatters] Add formatter for C++17 std::variant
rL342563: [DataFormatters] Add formatter for C++17 std::variant
rLLDB342421: [DataFormatters] Add formatter for C++17 std::variant
rL342421: [DataFormatters] Add formatter for C++17 std::variant
Diff Detail
Event Timeline
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? |
A bunch of little comments and two more substantial bits.
- 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.
- 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? |
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 Interesting I ran git clang-format before generating the diff and it made changes, so I am not sure what happened
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
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.
Lines 29-41 can all be done with:
(self.target, self.process, _, bkpt) = lldbutils.run_to_source_breakpoint(self, "break here", SBFileSpec("main.c"))