[PATCH] D75188: [NFC][DebugInfo] Refactor address advancing operations to share code

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D75188#1895417 <https://reviews.llvm.org/D75188#1895417>, @labath wrote:

> This isn't really my area, so I don't want to get too involved here, but it seems to me the code readability is going to suffer with all of the extra arguments being added to these (getOperationAdvance + advanceAddr) functions. They're just a bunch of locals from the `LineTable::parse` function -- maybe if these were lambdas inside that function, then they could capture most of these arguments implicitly? Or even create some sort of an object that would carry the state of the line program parsing machinery (like all the flags about reported warnings), and all of these things could be methods on it? That way, we might even be able to break down the `parse` function somehow, which is getting a bit unwieldy...


Yeah, I and a colleague who I discussed this with did have similar concerns, although we hadn't come up with a better solution yet. I did consider making these lambdas, although I'm not a massive fan of having big lambdas (no specific reason for that, just my personal feeling). There is a `ParsingState` struct already being passed around. Perhaps some of the local variables could be put in that (i.e. the error handler, the warning booleans and the table offset, perhaps even the opcode offset)? That would reduce `advanceAddr`'s parameter count to 4 or 5 from 7, and `getOperationAdvance` from 8 to 6 or 5, I believe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75188