[PATCH] D74204: [DebugInfo] Reject line tables of version > 5

Mon Feb 10 10:00:05 PST 2020

dblaikie added a comment.

In D74204#1866664 <https://reviews.llvm.org/D74204#1866664>, @jhenderson wrote:

> In D74204#1864663 <https://reviews.llvm.org/D74204#1864663>, @dblaikie wrote:
>
> > Might be worth a different strategy (either add to the end, and/or use stable names rather than numbers). But otherwise seems fine.
>
>
> I think I'm going to do a follow-up review to remove the numbers. Previously, all my new cases were added to the end, but this test case is strongly related to the other version ones, so I thought it should belong with it.

Sounds good to me.

>> Other thoughts: Probably print the unknown version number in decimal rather than hex?
> 
> Yeah, printing in decimal makes sense. I'd like to do that in a follow-up, as it's a change to a pre-existing message.

Sure sure.

>> Probably doesn't need both an assembly+dumper test and a unit test. One or the other (which ever is most convenient - the unit test looks simpler/more self contained & targeted) is fine by me - assuming the general codepath for error handling is already tested for the dumper, for instance. (ie: the changed code is exercised in the new test, doesn't mean all paths that reach that changed code need to be retested).
> 
> Seems okay to me (I'd be inclined to keep the unit test here). I've been uncertain where to draw the distinction between the two, if I'm honest, and a number of my other recent changes have added both. On a previous code base I worked on, it was generally expected that there were both unit tests and system (i.e. lit-equivalent) testing for any change, so I've tended to err on the side of over-testing rather than under-testing.

I'll admit I'm a bit fussy about testing - both too much and too little can be maintenance problems & LLVM's choices aren't necessarily the best, nor applicable to all other projects/types of code.

My basic rule of thumb is: "If it's unlikly/impossible to introduce a bug that will cause only this test to fail and no others, then this test is probably unnecessary/excessive". And while LLVM historically tends towards end-to-end testing (where the end-to-end might be quite small, using some targeted tool like llvm-dwarfdump, opt, etc) with relatively little unit testing - the latter can be the right tool for the job in some situations & this may be one of them. Where the end-to-end error handling path is tested, but a smaller/easier to read/etc targeted unit test can then exercise the various sources of errors, without having to test all the way through the end-to-end every for every one.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74204/new/

https://reviews.llvm.org/D74204