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

Mon Feb 10 03:23:31 PST 2020

jhenderson added a comment.

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.

> 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.

> 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74204