[Lldb-commits] [PATCH] D49271: Adding libc++ formattors for std::optional

[Jim Ingham via Phabricator via lldb-commits]

jingham accepted this revision.
jingham added a comment.

This looks fine.  I agree with Davide that putting some description of the type you are formatting in comments somewhere would be make things easier for somebody else who might have to fix this (or to you when you've totally forgotten how this worked a year from now...)

I'm okay with this if you fix the things Davide asked about (except the regex, that looks right to me.)



================
Comment at: packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py:8
+
+lldbinline.MakeInlineTest(__file__, globals(), [no_debug_info_test])
----------------
davide wrote:
> I do wonder if you need a decorator to indicate that this is a libcxx only test (and skip everywhere the library isn't supported).
Yes, you do need to mark this or you will get spurious failures at least on Windows, which turns off the libcxx category.


================
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:496
+                  "libc++ std::optional synthetic children",
+                  ConstString("^std::__(ndk)?1::optional<.+>(( )?&)?$"),
+                  stl_synth_flags, true);
----------------
davide wrote:
> I'm a little nervous about this regex, but I think it's fine and I'll let Jim take another look in case I missed something.
That looks right to me, it's catching the type or a reference to it.


https://reviews.llvm.org/D49271