jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.
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.
2. 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.
================
Comment at:
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/variant/TestDataFormatterLibcxxVariant.py:29-41
+ self.runCmd("file " + self.getBuildArtifact("a.out"),
CURRENT_EXECUTABLE_SET)
+
+ bkpt = self.target().FindBreakpointByID(
+ lldbutil.run_break_set_by_source_regexp(
+ self, "break here"))
+
+ self.runCmd("run", RUN_SUCCEEDED)
----------------
Lines 29-41 can all be done with:
(self.target, self.process, _, bkpt) = lldbutils.run_to_source_breakpoint(self,
"break here", SBFileSpec("main.c"))
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp:27
+// - __value refers to the actual value contained
+// - __tail which refers to the remaining pack types and
+//
----------------
I don't think you want the trailing "and" here.
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp:65
+// 1) VALID, we can obtain it and its not variant_npos
+// 2) INVALID, we can't obtian it or it is not a type we expect
+// 3) NPOS, its value is variant_npos which means the variant has no value
----------------
obtain
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp:84
+
+ // We are expecting to layers of typedefs before we obtain the basic type
+ // The next two checks verify this.
----------------
to -> two?
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp:92
+
+ lldb::BasicType index_basic_type =
index_compiler_type.GetTypedefedType().GetTypedefedType().GetBasicTypeEnumeration()
;
+
----------------
Sadly this needs to be wrapped to 80 characters somewhere.
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp:97-105
+ if ( index_basic_type == eBasicTypeUnsignedInt ) {
+ if( index_value == static_cast<unsigned int>(-1))
+ return LibcxxVariantIndexValidity::NPOS ;
+ } else if ( index_basic_type == eBasicTypeUnsignedLong ) {
+ if( index_value == static_cast<unsigned long>(-1))
+ return LibcxxVariantIndexValidity::NPOS ;
+ } else if ( index_basic_type == eBasicTypeUnsignedLongLong ) {
----------------
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.
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp:273
+
+ return head_value->Clone(ConstString(llvm::formatv("Value").str()));
+}
----------------
Do you need the "formatv" here? Looks like you are just making a ConstString
from "Value"? Maybe something got lost?
https://reviews.llvm.org/D51520
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
