[Lldb-commits] [PATCH] D49271: Adding libc++ formattors for std::optional
Jim Ingham via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Tue Jul 17 17:40:36 PDT 2018
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])
> 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);
> 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.
More information about the lldb-commits