[Lldb-commits] [PATCH] D114668: [lldb][NFC] Move generic DWARFASTParser code out of Clang-specific code

Tue Nov 30 14:20:01 PST 2021

clayborg added inline comments.

================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp:77
+          default:
+          case DW_AT_abstract_origin:
+          case DW_AT_accessibility:
----------------
shafik wrote:
> ljmf00 wrote:
> > bulbazord wrote:
> > > ljmf00 wrote:
> > > > Why we are including just these specific attributes? Maybe we should add a comment explaining it. According to the DWARF standard, any attribute is eligible for any tag.
> > > I'm not sure why. Possibly they were added to make sure the switch was fully covered (potentially to silence a warning)? You could add a `FIXME` or a `TODO` if you feel that these attributes should have functionality associated with them like the ones above.
> > I don't think it is to mark it as fully covered since there are much more attributes, the default label will address it anyway, and according to the DWARF standard, any attribute can be in a type tag (realistically, any tag). We can take the example of `DW_AT_description` which is just a description associated with the symbol. I feel like this can be safely deleted but I'm afraid to do it in favour of some other rationale I'm not seeing.
> @clayborg it looks like this has been this way since you put this in: https://github.com/llvm/llvm-project/commit/261ac3f4b5b98d02dd8718078015a92cf07df736
> 
> Do you agree this is dead code or is there something we are missing?
Fine to remove these extra attributes that are in the default case as we definitely don't have all of the attributes listed here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114668