mirror of
https://git.tartarus.org/simon/putty.git
synced 2025-04-11 08:08:06 -05:00
afb3dab1e9
In windows/window.c, a few variables inside functions were declared as static, with no particular purpose that I can see: they don't seem to have any reason to persist between calls to the function. So it makes more sense to have them be ordinary stack-allocated automatic variables. Static variables removed by this commit: - 'RECT ss' in reset_window. - 'WORD keys[3]' and 'BYTE keysb[3]' in TranslateKey. - several (buffer, length) pairs in do_text_internal. - keys_unicode[] in TranslateKey. All of these variables were originally introduced in patches credited to Robert de Bath, which means I can't even try to reconstruct my original thought processes, because they weren't _my_ thoughts anyway. The arrays in do_text_internal are the easiest to understand: they're reallocated larger as necessary, and making them static means the allocation from a previous call can be reused, saving a malloc (though I don't think that's a good enough reason to bother, these days). The fixed-size static arrays and RECT are harder to explain. I suspect they might originally have been that way because of 1990s attitudes to performance: in x86-32 it's probably marginally faster to give your variables constant addresses than sp-relative ones, and in the 1990s computers were much slower, so there's an argument for making things static if you have no _need_ to make them automatic. These days, the difference is negligible, and persistent state is much more widely recognised as a risk! But keys_unicode[] is by far the strangest, because there was code that clearly _did_ expect it to persist between calls, namely three assignments to keys_unicode[0] near the end of the function after it's finished being used for any other purpose, and a conditioned-out set of debug() calls at the top of the function that print its contents before anything has yet written to it. But as far as I can see, the persistent data in the array is otherwise completely unused. In any call to the function, if keys_unicode is used at all, then it's either written directly by a call to ToAsciiEx, or else (for pre-NT platforms) converted from ToAsciiEx's output via MultiByteToWideChar. In both cases, the integer variable 'r' indicates how many array elements were written, and subsequent accesses only ever read those elements. So the assignments to keys_unicode[0] at the end of the previous call will be overwritten before anything at all can depend on them - with the exception of those debug statements. I don't really understand what was going on here. It's tempting to guess that those final assignments must have once done something useful, and the code that used them was later removed. But the source control history doesn't bear that out: a static array of three elements (under its original name 'keys') was introduced in commit 0d5d39064a0d078, and then commits 953b7775b321a60 and 26f1085038d7cd9 added the other two assignments. And as far as I can see, even as of the original commit 0d5d39064a0d078, the code already had the property that there was a final assignment to keys[0] which would inevitably be overwritten in the next call before it could affect anything. So I'm totally confused about what those assignments were _ever_ useful for. But an email thread from the time suggests that some of those patches were being rebased repeatedly past other work (or rather, the much less reliable CVS analogue of rebasing), so my best guess is that that's where the confusion crept in - perhaps in RDB's original version of the code they did do something useful. Regardless of that, I'm pretty convinced that persistent array can't be doing anything useful _now_. So I'm taking it out. But if anyone reports a bug resulting from this change, then I'll eat my words - and with any luck the details of the bug report will give us a clue what's going on, and then we can put back some equivalent functionality with much better comments!