[Lldb-commits] [PATCH] D115662: [lldb][DWARF] Remove duplicate DIE type assignment

Thu Dec 30 13:09:54 PST 2021

ljmf00 planned changes to this revision.
ljmf00 added a comment.

In D115662#3211312 <https://reviews.llvm.org/D115662#3211312>, @labath wrote:

> I don't have any issues with this, if everything still works. I hoped there would be more of a discussion on this (I was hoping I'd maybe learn something from it). That obviously didn't happen, but I don't think it needs to hold this back.

I understand your point and I'm fine with discussing this further since this is not a blocker for me :)

I did this as an improvement as I saw this as a duplicate and apparently more duplicates of this assignment (see here https://github.com/llvm/llvm-project/blob/ed6c757d5c5926a72183cdd15a5b1efc54111db0/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp#L1552 ) while reading the code.

To give more context, I'm in the process of generalizing `UpdateSymbolContextScopeForType` to be able to use it on other non-Clang language plugins (I'm creating a D lldb plugin), see D115201 <https://reviews.llvm.org/D115201>. Both the `TypeList` and the DIEToType mapping are/were duplicated on `ParseTypeFromClangModule`. That said, `UpdateSymbolContextScopeForType` included both and it is useful to do that there due to generalization, although I also understand that `UpdateSymbolContextScopeForType` is targeted for different tasks.

I propose to either:

1. change the name of `UpdateSymbolContextScopeForType` to accommodate both insertions (reverting D115308 <https://reviews.llvm.org/D115308> and removing both assignments on `ParseTypeFromClangModule` and elsewhere)
2. create a separate routine to perform  both insertions (removing it from `UpdateSymbolContextScopeForType` and elsewhere)
3. simply delete them from `UpdateSymbolContextScopeForType` and keep it on `ParseTypeFromClangModule` and other places.

This patch is actually doing none of that since I didn't find the other duplicate assignments at the time of writing it.

I don't have the full picture of this, but this seems to be code that is needed independent of the DWARFASTParser. Looking at older implementations of language plugins (Go, for example), they copy and pasted `UpdateSymbolContextScopeForType` in their implementation (see https://github.com/llvm/llvm-project/commit/77198bc79b54267f2ce981c3a6c9c0d6384cac01#diff-0cf0246cf7e5ca0f0d43abe83aff7040786afb07703394799a5122cb6b3a18e7L428 ).

Note: I don't have active guidance on LLDB codebase, so I try to learn from the documentation and the actual codebase by myself. Maybe @zequanwu or @labath have more knowledge of the code and they can share their thoughts on this.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115662