labath wrote:
The code is fine, but you should really add a test for it. You don't need to
wait for lldb-test to grow symtab support. You can write a test that does
something like:
```
# RUN: yaml2obj %s > %t
# RUN: %lldb %t -o "image dump symtab" -o exit | FileCheck %s
# CHECK: symtab contents
```
If you grep for "image dump symtab", you can find some existing tests to
emulate.
I also feel I need to say something about comments. The most useful kind of
comment is the one that answers the "why" question. Most of your comments try
to answer the "what". Ideally the code should be clear enough that I can tell
*what* it is doing without reading the comment. The part that's often not
obvious is **why** is it doing that.
Taking this as an example:
```
// Remove the dot prefix from symbol names before adding to symtab. '.text'
// -> 'text'
```
The comment is not bad, but it's not that helpful as I can deduce that from the
code. When reading this, the questions I have are:
- why are we doing that?
- who adds the "." and why?
- what would happen if we didn't do that?
- etc.
This isn't a hard requirement for the patch, but please keep this in mind.
https://github.com/llvm/llvm-project/pull/141577
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
