DhruvSrivastavaX wrote: Got it @labath, I have added the updates accordingly. Hope this patch serves the purpose. 🙂
> 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
