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

[Luís Ferreira via Phabricator via lldb-commits]

ljmf00 added a comment.

In D112658#3092586 <https://reviews.llvm.org/D112658#3092586>, @labath wrote:

> Thanks for doing this. Just a couple of small remarks.

Can you re-review?



================
Comment at: lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp:35
 
-bool lldb_private::formatters::Char8StringSummaryProvider(
-    ValueObject &valobj, Stream &stream, const TypeSummaryOptions &) {
-  ProcessSP process_sp = valobj.GetProcessSP();
-  if (!process_sp)
-    return false;
+namespace {
+
----------------
labath wrote:
> We don't use [[ https://llvm.org/docs/CodingStandards.html#anonymous-namespaces | anonymous namespaces]] like this. `static` is sufficient.
Done


================
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) {
----------------
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.


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

https://reviews.llvm.org/D112658