[Lldb-commits] [PATCH] D72133: Data formatters: Look through array element typedefs

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin marked 3 inline comments as done.
jarin added inline comments.


================
Comment at: lldb/source/API/SBType.cpp:215-218
+  CompilerType canonical_type =
+      m_opaque_sp->GetCompilerType(true).GetCanonicalType();
+  return LLDB_RECORD_RESULT(
+      SBType(TypeImplSP(new TypeImpl(canonical_type.GetArrayElementType()))));
----------------
clayborg wrote:
> Why does getting the canonical type of an array type change the array element type? I would think it wouldn't matter but it obviously must?
Removal of typedef for element type is the existing behavior that I have been trying preserve. Raphael already suggested we should not preserve it.

For example, I get the following output for my program (both without and with my patch):

```
Type:    MCHAR [10]
El type: char
```

Here is the script

```
import lldb, os

debugger = lldb.SBDebugger.Create()
debugger.SetAsync(False)
target = debugger.CreateTargetWithFileAndTargetTriple("df.out", \
  "x86_64-pc-linux")
main_bp = target.BreakpointCreateByLocation("df.cc", 11)
process = target.LaunchSimple(None, None, os.getcwd())
frame = process.selected_thread.GetSelectedFrame()
var_a = process.selected_thread.GetSelectedFrame().GetValueForVariablePath("a")
print("Type:    " + var_a.GetType().GetName())
print("El type: " + var_a.GetType().GetArrayElementType().GetName())
```

If we remove the canonicalization here, we will indeed get the behavior that you want:

```
Type:    MCHAR [10]
El type: MCHAR
```




================
Comment at: lldb/source/DataFormatters/FormatManager.cpp:245-261
+  uint64_t array_size;
+  if (compiler_type.IsArrayType(nullptr, &array_size, nullptr)) {
+    CompilerType element_type = compiler_type.GetArrayElementType();
+    if (element_type.IsTypedefType()) {
+      // Get the stripped element type and compute the stripped array type
+      // from it.
+      CompilerType deffed_array_type =
----------------
clayborg wrote:
> Wouldn't we want to add one for each typedef that has a formatter? Lets say we had:
> 
> ```
> typedef char MCHAR;
> typedef MCHAR MMCHAR;
> 
> int main() {
>   MMCHAR str[5] = "abcd";
>   return 0;
> }
> ```
> 
> if we had a special formatter for MCHAR that would maybe display MCHAR somehow special, would we be ok with always getting the standard C string formatter in this case? Seems like we should add one for each typedef that has a formatter and possibly also the base type?
This does not work as expected with my patch, let me investigate why.


================
Comment at: lldb/source/Symbol/ClangASTContext.cpp:3946
 
-    CompilerType element_type =
-        GetType(array_eletype->getCanonicalTypeUnqualified());
+    CompilerType element_type = GetType(clang::QualType(array_eletype, 0));
 
----------------
clayborg wrote:
> If I ask an array type for its element type, I wouldn't expect all typedefs to be removed. This seems wrong, or it can become an option to this function as an extra param:
> 
> ```
> CompilerType
> ClangASTContext::GetArrayElementType(lldb::opaque_compiler_type_t type, uint64_t *stride, bool get_canonical_element_type);
> ```
The original code for ClangASTContext::GetArrayElementType did remove the typedefs. With my change ClangASTContext::GetArrayElementType does not remove typedefs anymore.


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

https://reviews.llvm.org/D72133