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

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

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



================
Comment at: packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/variant/main.cpp:29
+    std::variant<int, double, char> v3;
+    std::variant<int, double, char> v_no_value;
+
----------------
vsk wrote:
> Does a std::variant containing a std::variant work?
Yes, adding test.


================
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 ) {
----------------
jingham wrote:
> 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.  
I believe I applied the change as we discussed earlier, let me know if not.


https://reviews.llvm.org/D51520