[Lldb-commits] [PATCH] D153489: [lldb] Print hint if object description is requested but not implemented

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.


================
Comment at: lldb/source/Commands/CommandObjectDWIMPrint.cpp:81-83
+        << "note: object description requested, but type doesn't implement "
+           "a custom object description. Consider using \"p\" instead of "
+           "\"po\"\n";
----------------
I wonder if there are users who might ignore this, and then find it annoying to see it repeatedly. Should it be a once per session warning? Once per class? Or controlled by a setting.


================
Comment at: lldb/source/Commands/CommandObjectDWIMPrint.cpp:153-154
 
-      valobj_sp->Dump(result.GetOutputStream(), dump_options);
+      MaybeAddPoHintAndDump(*valobj_sp.get(), dump_options, language, is_po,
+                            result.GetOutputStream());
+
----------------
I know it would be less "DRY", but it also feels weird to me to pass in a bool that controls whether the function does anything. At the callsite, I think the semantics would be more clear as:

```
if (is_po)
    MaybeAddPoHintAndDump(*valobj_sp.get(), dump_options, language, 
                          result.GetOutputStream());
```


================
Comment at: lldb/source/Commands/CommandObjectDWIMPrint.cpp:177
       if (valobj_sp->GetError().GetError() != UserExpression::kNoResult)
-        valobj_sp->Dump(result.GetOutputStream(), dump_options);
+        MaybeAddPoHintAndDump(*valobj_sp.get(), dump_options, language, is_po,
+                              result.GetOutputStream());
----------------



================
Comment at: lldb/source/Commands/CommandObjectDWIMPrint.h:46-51
+  /// Add a hint if object description was requested, but no description
+  /// function was implemented, and dump valobj to output_stream after.
+  static void MaybeAddPoHintAndDump(ValueObject &valobj,
+                                    const DumpValueObjectOptions &dump_options,
+                                    lldb::LanguageType language, bool is_po,
+                                    Stream &output_stream);
----------------
Should this hint be part of dwim-print only? At this point I see `expression` as a command you use to do exactly what you ask it to do.


================
Comment at: lldb/source/Commands/CommandObjectFrame.cpp:708
                 rec_value_sp->GetPreferredDisplayLanguage());
+            rec_value_sp->GetPreferredDisplayLanguage();
             options.SetRootValueObjectName(rec_value_sp->GetName().AsCString());
----------------
Why is this line added? If this getter has side effects, we should create an appropriately named function that performs the side effects.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153489