[Lldb-commits] [PATCH] D81119: [lldb] Fix SLEB128 decoding

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