[Lldb-commits] [PATCH] D81119: [lldb] Fix SLEB128 decoding
Pavel Labath via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Mon Jun 8 02:40:56 PDT 2020
labath added a comment.
In D81119#2074109 <https://reviews.llvm.org/D81119#2074109>, @jankratochvil wrote:
> In D81119#2073973 <https://reviews.llvm.org/D81119#2073973>, @labath wrote:
>
> > PS: You can still just drop the test if you think that's too much hassle. :)
>
>
> It is sure nice to learn more the LLDB high standards, thanks for the review.
Just to clarify. These are "standards" that I try to set for my self -- I think it's important to be able to quickly ascertain what a test does, and needing to skip over irreletant attributes or follow indirect references makes that hard (DWARF v5 makes this particularly bad as it introduces a lot more nonsymbolic reference via all DW_FORM_***x encodings). Ideally, then it would not be necessary to put a comment saying "this file declares a single long variable with the value 47" because that would be sufficiently obvious without that. Right now, we're pretty far from that, but there's a GSoC project http://lists.llvm.org/pipermail/llvm-dev/2020-May/141690.html, which I'm hoping will get us a step towards that.
The reason I chose to do that here is because I you seemed very motivated to get that test in, and I considered it very optional and the only meaning I could see in it was if it was polished very well. (After the first round of comments, I was expecting you would just delete that test). I do not insist on this level of reduction in most tests -- I am usually happy that there is a test in the first place.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81119/new/
https://reviews.llvm.org/D81119
More information about the lldb-commits
mailing list