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

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

Reply via email to