[Lldb-commits] [PATCH] D112658: [lldb] Refactor C/C++ string and char summary providers

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Looks good.

Do you have commit access, or should I check this in for you (I can also fix up the formatting in that case)?



================
Comment at: lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp:37-40
+static constexpr std::pair<const char*, Format> getElementTraits(StringElementType ElemType)
+{
+  switch(ElemType)
+  {
----------------
This isn't the right formatting (brackets don't get their own line, and the first line is too long). Could you run the patch through clang-format please?


================
Comment at: lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp:76-85
+    if (ElemType == StringPrinter::StringElementType::UTF8) {
+      options.SetPrefixToken("u8");
+      valobj.GetValueAsCString(lldb::eFormatUnicode8, value);
+    } else if (ElemType == StringPrinter::StringElementType::UTF16) {
+      options.SetPrefixToken("u");
+      valobj.GetValueAsCString(lldb::eFormatUnicode16, value);
+    } else if (ElemType == StringPrinter::StringElementType::UTF32) {
----------------
ljmf00 wrote:
> labath wrote:
> > Maybe a helper function like `pair<StringRef, Format> getElementTraits(StringElementType)` would reduce the repetition further? Or possibly it could be a static array indexed by `ElemType`.
> Done. I used `constexpr auto`, I'm not sure if `auto` is a practice here in the LLVM codebase or I should explicitly specify the pair type.
I'd probably spell out the type (and it looks like you already did).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112658/new/

https://reviews.llvm.org/D112658