[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/include/lldb/API/SBType.h:212
+  /// Returns true for types that were incomplete in the debug information but
+  /// should have been a complete. When the debugger constructs types, we must
+  /// have enough information to reconstruct a type in a language specific AST
----------------
aprantl wrote:
> a complete? (missing word)
yep, will remove the "a" here..


================
Comment at: lldb/include/lldb/API/SBType.h:223
+  /// of the debug information in a debug session, possibly even in another
+  /// executable or shared library's debug information. If we require a full
+  /// definition for a type but we can't find ony, we must forcefully complete
----------------
aprantl wrote:
> Could you replace all instances of "we" with something more concrete? It sounds like this paragraph really describes behaviors of TypeSystemClang?
It is describing what -flimit-debug-info does, so yes, this is very clang specific. But right now we are showing these omitted types to the user as if they are fine, and they are not, so the user and or UI can and should convey this somehow. Not a great user experience when someone enables -flimit-debug-info, not on purpose as this is the default on non darwin targets, and they get a subpar debugging experience. We run into this all the time with linux and Android and the users think the debugger doesn't know how to show variables, so we need to let them know what is going on somehow. 

That being said, I can probably make this paragraph a lot simpler. Open to suggestions.


================
Comment at: lldb/include/lldb/API/SBType.h:237
+  /// need to be forcefully completed
+  bool IsTypeForcefullyCompleted();
+
----------------
aprantl wrote:
> Why not `IsIncomplete()`?
We already have IsTypeComplete() above, which indicates that something is a forward declaration. The IsTypeComplete() will return true for these forcefully completed types since they appear to be complete. I am open to suggestions on better naming, I was just forwarding our internal function names, but for the API a better name might make more sense. I can't think of any off hand.


================
Comment at: lldb/source/Core/ValueObject.cpp:601
+  if (GetCompilerType().IsForcefullyCompleted()) {
+      destination = "<incomplete type>";
+      return true;
----------------
aprantl wrote:
> I don't ha
I am open to suggestions on what people want this string to be. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259