[Lldb-commits] [PATCH] D146783: [lldb] Add ability to hide the root name of a value

2023-03-27 Thread Dave Lee via Phabricator via lldb-commits
kastiglione added a comment.

@jgorbe thanks for reporting. I have a fix for the missing space, I will look 
into the cause of the double space and fix that as well. I'll do this today.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146783

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D146783: [lldb] Add ability to hide the root name of a value

2023-03-24 Thread Jorge Gorbe Moya via Phabricator via lldb-commits
jgorbe added a comment.

I just noticed a minor aesthetic problem with the input of `dwim-print` when 
using data formatters. There are some spacing adjustments in this commit but 
I'm not sure if they are the actual cause (please let me know if you'd prefer 
me to file a bug instead).

You can reproduce it by defining some random struct `X` and using `settings set 
auto-one-line-summaries false` like you did in the test case for this commit.

If you add a summary string:

  type summary add -s "my summary" -e "X"

then the output of dwim-print without persistent results has the wrong spacing:

  (lldb) dwim-print my_x
  (X)  my summary{
value = 0
  }

(note there are two spaces before "my summary" and no space before the opening 
brace).

Using `expr` or `dwim-print --persistent-result on --` doesn't have this 
problem:

  (lldb) dwim-print --persistent-result on -- my_x
  (X) $2 = my summary {
value = 0
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146783

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D146783: [lldb] Add ability to hide the root name of a value

2023-03-24 Thread Dave Lee via Phabricator via lldb-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rG23349d83a98f: [lldb] Add ability to hide the root name of a 
value (authored by kastiglione).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146783

Files:
  lldb/include/lldb/DataFormatters/DumpValueObjectOptions.h
  lldb/include/lldb/DataFormatters/ValueObjectPrinter.h
  lldb/source/Commands/CommandObjectDWIMPrint.cpp
  lldb/source/Commands/CommandObjectExpression.cpp
  lldb/source/DataFormatters/DumpValueObjectOptions.cpp
  lldb/source/DataFormatters/ValueObjectPrinter.cpp
  lldb/test/API/commands/dwim-print/TestDWIMPrint.py
  lldb/test/API/commands/dwim-print/main.c

Index: lldb/test/API/commands/dwim-print/main.c
===
--- lldb/test/API/commands/dwim-print/main.c
+++ lldb/test/API/commands/dwim-print/main.c
@@ -1,3 +1,10 @@
+struct Structure {
+  int number;
+};
+
 int main(int argc, char **argv) {
+  struct Structure s;
+  s.number = 30;
+  // break here
   return 0;
 }
Index: lldb/test/API/commands/dwim-print/TestDWIMPrint.py
===
--- lldb/test/API/commands/dwim-print/TestDWIMPrint.py
+++ lldb/test/API/commands/dwim-print/TestDWIMPrint.py
@@ -114,3 +114,11 @@
 error_msg = "error: 'dwim-print' takes a variable or expression"
 self.expect(f"dwim-print", error=True, startstr=error_msg)
 self.expect(f"dwim-print -- ", error=True, startstr=error_msg)
+
+def test_nested_values(self):
+"""Test dwim-print with nested values (structs, etc)."""
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.c"))
+self.runCmd("settings set auto-one-line-summaries false")
+self._expect_cmd(f"dwim-print s", "frame variable")
+self._expect_cmd(f"dwim-print (struct Structure)s", "expression")
Index: lldb/source/DataFormatters/ValueObjectPrinter.cpp
===
--- lldb/source/DataFormatters/ValueObjectPrinter.cpp
+++ lldb/source/DataFormatters/ValueObjectPrinter.cpp
@@ -275,7 +275,7 @@
 
   StreamString varName;
 
-  if (!m_options.m_hide_name) {
+  if (ShouldShowName()) {
 if (m_options.m_flat_output)
   m_valobj->GetExpressionPath(varName);
 else
@@ -314,7 +314,7 @@
   m_stream->Printf("(%s) ", typeName.GetData());
 if (!varName.Empty())
   m_stream->Printf("%s =", varName.GetData());
-else if (!m_options.m_hide_name)
+else if (ShouldShowName())
   m_stream->Printf(" =");
   }
 }
@@ -437,7 +437,7 @@
 if (m_options.m_hide_pointer_value &&
 IsPointerValue(m_valobj->GetCompilerType())) {
 } else {
-  if (!m_options.m_hide_name)
+  if (ShouldShowName())
 m_stream->PutChar(' ');
   m_stream->PutCString(m_value);
   value_printed = true;
@@ -459,7 +459,7 @@
 // let's avoid the overly verbose no description error for a nil thing
 if (m_options.m_use_objc && !IsNil() && !IsUninitialized() &&
 (!m_options.m_pointer_as_array)) {
-  if (!m_options.m_hide_value || !m_options.m_hide_name)
+  if (!m_options.m_hide_value || ShouldShowName())
 m_stream->Printf(" ");
   const char *object_desc = nullptr;
   if (value_printed || summary_printed)
@@ -565,8 +565,14 @@
 if (ShouldPrintValueObject())
   m_stream->EOL();
   } else {
-if (ShouldPrintValueObject())
-  m_stream->PutCString(IsRef() ? ": {\n" : " {\n");
+if (ShouldPrintValueObject()) {
+  if (IsRef()) {
+m_stream->PutCString(": ");
+  } else if (ShouldShowName()) {
+m_stream->PutChar(' ');
+  }
+  m_stream->PutCString("{\n");
+}
 m_stream->IndentMore();
   }
 }
@@ -819,3 +825,9 @@
 bool ValueObjectPrinter::HasReachedMaximumDepth() {
   return m_curr_depth >= m_options.m_max_depth;
 }
+
+bool ValueObjectPrinter::ShouldShowName() const {
+  if (m_curr_depth == 0)
+return !m_options.m_hide_root_name && !m_options.m_hide_name;
+  return !m_options.m_hide_name;
+}
Index: lldb/source/DataFormatters/DumpValueObjectOptions.cpp
===
--- lldb/source/DataFormatters/DumpValueObjectOptions.cpp
+++ lldb/source/DataFormatters/DumpValueObjectOptions.cpp
@@ -19,10 +19,10 @@
   m_decl_printing_helper(), m_pointer_as_array(), m_use_synthetic(true),
   m_scope_already_checked(false), m_flat_output(false), m_ignore_cap(false),
   m_show_types(false), m_show_location(false), m_use_objc(false),
-  m_hide_root_type(false), m_hide_name(false), m_hide_value(false),
-  m_run_validator(false), m_use_type_display_name(true),
-  m_allow_oneliner_mode(true), m_hide_pointer_value(false),
-  m_reveal_empty_aggregates(true) {}
+  

[Lldb-commits] [PATCH] D146783: [lldb] Add ability to hide the root name of a value

2023-03-24 Thread Dave Lee via Phabricator via lldb-commits
kastiglione added inline comments.



Comment at: lldb/source/DataFormatters/ValueObjectPrinter.cpp:278
 
-  if (!m_options.m_hide_name) {
+  if (ShowName()) {
 if (m_options.m_flat_output)

jgorbe wrote:
> This method name reads like a command, rather than a predicate. What about 
> something like `ShouldShowName` or `ShouldPrintName`? This would also match 
> the style of other method names already in this file such as 
> `ShouldPrintValueObject`.
Good call, ShouldShowName is better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146783

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D146783: [lldb] Add ability to hide the root name of a value

2023-03-24 Thread Dave Lee via Phabricator via lldb-commits
kastiglione updated this revision to Diff 508173.
kastiglione added a comment.

Rename ShowName to ShouldShowName


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146783

Files:
  lldb/include/lldb/DataFormatters/DumpValueObjectOptions.h
  lldb/include/lldb/DataFormatters/ValueObjectPrinter.h
  lldb/source/Commands/CommandObjectDWIMPrint.cpp
  lldb/source/Commands/CommandObjectExpression.cpp
  lldb/source/DataFormatters/DumpValueObjectOptions.cpp
  lldb/source/DataFormatters/ValueObjectPrinter.cpp
  lldb/test/API/commands/dwim-print/TestDWIMPrint.py
  lldb/test/API/commands/dwim-print/main.c

Index: lldb/test/API/commands/dwim-print/main.c
===
--- lldb/test/API/commands/dwim-print/main.c
+++ lldb/test/API/commands/dwim-print/main.c
@@ -1,3 +1,10 @@
+struct Structure {
+  int number;
+};
+
 int main(int argc, char **argv) {
+  struct Structure s;
+  s.number = 30;
+  // break here
   return 0;
 }
Index: lldb/test/API/commands/dwim-print/TestDWIMPrint.py
===
--- lldb/test/API/commands/dwim-print/TestDWIMPrint.py
+++ lldb/test/API/commands/dwim-print/TestDWIMPrint.py
@@ -114,3 +114,11 @@
 error_msg = "error: 'dwim-print' takes a variable or expression"
 self.expect(f"dwim-print", error=True, startstr=error_msg)
 self.expect(f"dwim-print -- ", error=True, startstr=error_msg)
+
+def test_nested_values(self):
+"""Test dwim-print with nested values (structs, etc)."""
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.c"))
+self.runCmd("settings set auto-one-line-summaries false")
+self._expect_cmd(f"dwim-print s", "frame variable")
+self._expect_cmd(f"dwim-print (struct Structure)s", "expression")
Index: lldb/source/DataFormatters/ValueObjectPrinter.cpp
===
--- lldb/source/DataFormatters/ValueObjectPrinter.cpp
+++ lldb/source/DataFormatters/ValueObjectPrinter.cpp
@@ -275,7 +275,7 @@
 
   StreamString varName;
 
-  if (!m_options.m_hide_name) {
+  if (ShouldShowName()) {
 if (m_options.m_flat_output)
   m_valobj->GetExpressionPath(varName);
 else
@@ -314,7 +314,7 @@
   m_stream->Printf("(%s) ", typeName.GetData());
 if (!varName.Empty())
   m_stream->Printf("%s =", varName.GetData());
-else if (!m_options.m_hide_name)
+else if (ShouldShowName())
   m_stream->Printf(" =");
   }
 }
@@ -437,7 +437,7 @@
 if (m_options.m_hide_pointer_value &&
 IsPointerValue(m_valobj->GetCompilerType())) {
 } else {
-  if (!m_options.m_hide_name)
+  if (ShouldShowName())
 m_stream->PutChar(' ');
   m_stream->PutCString(m_value);
   value_printed = true;
@@ -459,7 +459,7 @@
 // let's avoid the overly verbose no description error for a nil thing
 if (m_options.m_use_objc && !IsNil() && !IsUninitialized() &&
 (!m_options.m_pointer_as_array)) {
-  if (!m_options.m_hide_value || !m_options.m_hide_name)
+  if (!m_options.m_hide_value || ShouldShowName())
 m_stream->Printf(" ");
   const char *object_desc = nullptr;
   if (value_printed || summary_printed)
@@ -565,8 +565,14 @@
 if (ShouldPrintValueObject())
   m_stream->EOL();
   } else {
-if (ShouldPrintValueObject())
-  m_stream->PutCString(IsRef() ? ": {\n" : " {\n");
+if (ShouldPrintValueObject()) {
+  if (IsRef()) {
+m_stream->PutCString(": ");
+  } else if (ShouldShowName()) {
+m_stream->PutChar(' ');
+  }
+  m_stream->PutCString("{\n");
+}
 m_stream->IndentMore();
   }
 }
@@ -819,3 +825,9 @@
 bool ValueObjectPrinter::HasReachedMaximumDepth() {
   return m_curr_depth >= m_options.m_max_depth;
 }
+
+bool ValueObjectPrinter::ShouldShowName() const {
+  if (m_curr_depth == 0)
+return !m_options.m_hide_root_name && !m_options.m_hide_name;
+  return !m_options.m_hide_name;
+}
Index: lldb/source/DataFormatters/DumpValueObjectOptions.cpp
===
--- lldb/source/DataFormatters/DumpValueObjectOptions.cpp
+++ lldb/source/DataFormatters/DumpValueObjectOptions.cpp
@@ -19,10 +19,10 @@
   m_decl_printing_helper(), m_pointer_as_array(), m_use_synthetic(true),
   m_scope_already_checked(false), m_flat_output(false), m_ignore_cap(false),
   m_show_types(false), m_show_location(false), m_use_objc(false),
-  m_hide_root_type(false), m_hide_name(false), m_hide_value(false),
-  m_run_validator(false), m_use_type_display_name(true),
-  m_allow_oneliner_mode(true), m_hide_pointer_value(false),
-  m_reveal_empty_aggregates(true) {}
+  m_hide_root_type(false), m_hide_root_name(false), m_hide_name(false),
+  

[Lldb-commits] [PATCH] D146783: [lldb] Add ability to hide the root name of a value

2023-03-24 Thread Jorge Gorbe Moya via Phabricator via lldb-commits
jgorbe added a comment.

I've tried building lldb with this patch applied and it fixes the problem I 
saw. Thanks for the quick fix!

I'm not very familiar with the code so I'd appreciate if someone else gives 
their LGTM too but the changes look reasonable to me (just one small nit I've 
mentioned in an inline comment).




Comment at: lldb/source/DataFormatters/ValueObjectPrinter.cpp:278
 
-  if (!m_options.m_hide_name) {
+  if (ShowName()) {
 if (m_options.m_flat_output)

This method name reads like a command, rather than a predicate. What about 
something like `ShouldShowName` or `ShouldPrintName`? This would also match the 
style of other method names already in this file such as 
`ShouldPrintValueObject`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146783

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D146783: [lldb] Add ability to hide the root name of a value

2023-03-23 Thread Dave Lee via Phabricator via lldb-commits
kastiglione created this revision.
kastiglione added reviewers: aprantl, jingham, jgorbe.
Herald added a project: All.
kastiglione requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

When printing a value, allow the root value's name to be elided, without 
omiting the
names of child values.

At the API level, this adds `SetHideRootName()`, which joins the existing
`SetHideName()` function.

This functionality is used by `dwim-print` and `expression`.

Fixes an issue identified by @jgorbe in https://reviews.llvm.org/D145609.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146783

Files:
  lldb/include/lldb/DataFormatters/DumpValueObjectOptions.h
  lldb/include/lldb/DataFormatters/ValueObjectPrinter.h
  lldb/source/Commands/CommandObjectDWIMPrint.cpp
  lldb/source/Commands/CommandObjectExpression.cpp
  lldb/source/DataFormatters/DumpValueObjectOptions.cpp
  lldb/source/DataFormatters/ValueObjectPrinter.cpp
  lldb/test/API/commands/dwim-print/TestDWIMPrint.py
  lldb/test/API/commands/dwim-print/main.c

Index: lldb/test/API/commands/dwim-print/main.c
===
--- lldb/test/API/commands/dwim-print/main.c
+++ lldb/test/API/commands/dwim-print/main.c
@@ -1,3 +1,10 @@
+struct Structure {
+  int number;
+};
+
 int main(int argc, char **argv) {
+  struct Structure s;
+  s.number = 30;
+  // break here
   return 0;
 }
Index: lldb/test/API/commands/dwim-print/TestDWIMPrint.py
===
--- lldb/test/API/commands/dwim-print/TestDWIMPrint.py
+++ lldb/test/API/commands/dwim-print/TestDWIMPrint.py
@@ -114,3 +114,11 @@
 error_msg = "error: 'dwim-print' takes a variable or expression"
 self.expect(f"dwim-print", error=True, startstr=error_msg)
 self.expect(f"dwim-print -- ", error=True, startstr=error_msg)
+
+def test_nested_values(self):
+"""Test dwim-print with nested values (structs, etc)."""
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.c"))
+self.runCmd("settings set auto-one-line-summaries false")
+self._expect_cmd(f"dwim-print s", "frame variable")
+self._expect_cmd(f"dwim-print (struct Structure)s", "expression")
Index: lldb/source/DataFormatters/ValueObjectPrinter.cpp
===
--- lldb/source/DataFormatters/ValueObjectPrinter.cpp
+++ lldb/source/DataFormatters/ValueObjectPrinter.cpp
@@ -275,7 +275,7 @@
 
   StreamString varName;
 
-  if (!m_options.m_hide_name) {
+  if (ShowName()) {
 if (m_options.m_flat_output)
   m_valobj->GetExpressionPath(varName);
 else
@@ -314,7 +314,7 @@
   m_stream->Printf("(%s) ", typeName.GetData());
 if (!varName.Empty())
   m_stream->Printf("%s =", varName.GetData());
-else if (!m_options.m_hide_name)
+else if (ShowName())
   m_stream->Printf(" =");
   }
 }
@@ -437,7 +437,7 @@
 if (m_options.m_hide_pointer_value &&
 IsPointerValue(m_valobj->GetCompilerType())) {
 } else {
-  if (!m_options.m_hide_name)
+  if (ShowName())
 m_stream->PutChar(' ');
   m_stream->PutCString(m_value);
   value_printed = true;
@@ -459,7 +459,7 @@
 // let's avoid the overly verbose no description error for a nil thing
 if (m_options.m_use_objc && !IsNil() && !IsUninitialized() &&
 (!m_options.m_pointer_as_array)) {
-  if (!m_options.m_hide_value || !m_options.m_hide_name)
+  if (!m_options.m_hide_value || ShowName())
 m_stream->Printf(" ");
   const char *object_desc = nullptr;
   if (value_printed || summary_printed)
@@ -565,8 +565,14 @@
 if (ShouldPrintValueObject())
   m_stream->EOL();
   } else {
-if (ShouldPrintValueObject())
-  m_stream->PutCString(IsRef() ? ": {\n" : " {\n");
+if (ShouldPrintValueObject()) {
+  if (IsRef()) {
+m_stream->PutCString(": ");
+  } else if (ShowName()) {
+m_stream->PutChar(' ');
+  }
+  m_stream->PutCString("{\n");
+}
 m_stream->IndentMore();
   }
 }
@@ -819,3 +825,9 @@
 bool ValueObjectPrinter::HasReachedMaximumDepth() {
   return m_curr_depth >= m_options.m_max_depth;
 }
+
+bool ValueObjectPrinter::ShowName() const {
+  if (m_curr_depth == 0)
+return !m_options.m_hide_root_name && !m_options.m_hide_name;
+  return !m_options.m_hide_name;
+}
Index: lldb/source/DataFormatters/DumpValueObjectOptions.cpp
===
--- lldb/source/DataFormatters/DumpValueObjectOptions.cpp
+++ lldb/source/DataFormatters/DumpValueObjectOptions.cpp
@@ -19,10 +19,10 @@
   m_decl_printing_helper(), m_pointer_as_array(), m_use_synthetic(true),
   m_scope_already_checked(false), m_flat_output(false), m_ignore_cap(false),
   m_show_types(false),