[PATCH] D49676: [DWARF] support for .debug_addr (consumer)

[Victor Leschuk via Phabricator via llvm-commits]

vleschuk added a comment.

In https://reviews.llvm.org/D49676#1182470, @jhenderson wrote:

> Okay, I think I'm happy with this, but please let the others give their feedback and thumbs up too.


@aprantl , @dblaikie , @probinson, do you have any objections on this?

> Have you considered writing unit tests for this at all? I don't think they're strictly necessary, but they would be nice.

I thought about it, I will give it more thought when we have more mature implementation.



================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:95-98
+  // FIXME: For now we just treat version mismatch as error,
+  // however the correct way to associate a .debug_addr table
+  // with a .debug_info table is to look at the DW_AT_addr_base
+  // attribute in the info table.
----------------
jhenderson wrote:
> I'd move this FIXME to where the UnitVersion is calculated in DWARFContext.cpp.
> 
> I also think the last two lines of this comment make no sense on their own, and can probably just be deleted.
> 
> Nit: as error, however the -> as an error. However, the
> 
> Aside: you don't need to manually wrap comments, because clang-format will do it for you at the correct column width.
> I'd move this FIXME to where the UnitVersion is calculated in DWARFContext.cpp.

I think it better to leave it here where the actual comparison is performed.

> I also think the last two lines of this comment make no sense on their own, and can probably just be deleted.

Agreed.

> Aside: you don't need to manually wrap comments, because clang-format will do it for you at the correct column width.

Yep, sorry, always forget to use this tool, need to hotkey it to my vimrc =)


https://reviews.llvm.org/D49676