[Lldb-commits] [PATCH] D51520: Add libc++ data formatter for std::variant

Jim Ingham via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Mon Sep 10 17:04:51 PDT 2018


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





More information about the lldb-commits mailing list