On Tue, Sep 11, 2018 at 8:22 PM, Huw Davies huw@codeweavers.com wrote:
Specifically the gotos, but I've already commented on the high identation level in this function (I know that's not your fault).
It seems to me that you want the WM_KEYDOWN handler to call the WM_CHAR handler. At the risk of repeating myself, having the handlers as a helper functions would make this easier.
Huw.
I honestly don't think the gotos are too bad (especially the VK_DELETE one since it's also very close), and my reasoning for it is because all the variables that could be affected are set right before the goto (ret, displayall, noautoappend), so there's no state to memorize, because every other variable is set before both switch statements (including parameters).
However, that probably won't convince you, so I'll maybe end up having to rewrite this, but would a larger patch be acceptable then? I mean, to get it in a "workable" state it's pretty hard to split it up.
That said there are still some difficulties with functions instead of goto, since the goto doesn't jump directly into WM_CHAR but a sub-block depending on condition. It is easy to make most of WM_CHAR a separate function, but not the original if blocks. For example, VK_DELETE's wParam is actually not a "control character" in terms of WM_CHAR's wParam, yet it needs to use that same code path (that's why it jumps inside the if statement, to share the code). Maybe I can give an extra bool argument to the helper function, to know whether to go the "control character" route or not. Definitely not as elegant in my opinion, but if it has to be done heh.
But yeah, I won't be able to do this without rewriting it...
BTW just wondering but what is exactly bad about the goto in this situation? (just the first one or both?) Of course, I know it's not ideal, but what makes it so bad I mean? I'm one of those who dislikes littering code with BOOLs and have a lot of checks just to avoid some gotos (which make code cleaner in that situation), so it's not obvious to me due to that background, that's why I'd like to know :-)