simon-git: putty (main): Simon Tatham
Commits to Tartarus hosted VCS
tartarus-commits at lists.tartarus.org
Sat May 27 17:49:36 BST 2023
TL;DR:
afb3dab1 Remove some pointless 'static' qualifiers.
322984d6 do_text_internal: fix bug in the lpDx_maybe mechanism.
6aca7f1e windows/window.c: move more variables into WinGuiSeat.
Repository: https://git.tartarus.org/simon/putty.git
On the web: https://git.tartarus.org/?p=simon/putty.git
Branch updated: main
Committer: Simon Tatham <anakin at pobox.com>
Date: 2023-05-27 17:49:36
commit afb3dab1e97a09fefe3f045e494f29c4f3a1ea26
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=afb3dab1e97a09fefe3f045e494f29c4f3a1ea26;hp=14d47544adaf1185be5c9c47d19330ddcaf31886
Author: Simon Tatham <anakin at pobox.com>
Date: Sat May 27 14:56:31 2023 +0100
Remove some pointless 'static' qualifiers.
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!
windows/window.c | 74 ++++++++++++++++++--------------------------------------
1 file changed, 24 insertions(+), 50 deletions(-)
commit 322984d635c9fd8db33b60be09edd126fcfe9e54
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=322984d635c9fd8db33b60be09edd126fcfe9e54;hp=afb3dab1e97a09fefe3f045e494f29c4f3a1ea26
Author: Simon Tatham <anakin at pobox.com>
Date: Sat May 27 15:26:38 2023 +0100
do_text_internal: fix bug in the lpDx_maybe mechanism.
lpDx_maybe was a pointer defined to point at either lpDx itself or
NULL, depending on whether the code decided it needed to pass the lpDx
array of per-character pixel offsets to various functions during
drawing (based in turn on whether the font was variable-pitch).
lpDx is reallocated as necessary, which means lpDx_maybe must be kept
up to date. This was achieved by resetting it to lpDx if it was
already non-NULL.
But lpDx starts out as NULL before the first reallocation, so that
this can't work - it'll be initialised to NULL even if we _did_ want
to use it, and then at the first realloc, it won't be updated!
Before the previous commit turned lpDx from a static into an automatic
variable, this would have been a rare bug affecting only the first
call to the function. Now it will happen all the time, which is
better, because we can notice and fix it.
Replaced lpDx_maybe completely with a boolean flag indicating whether
we should pass lpDx to drawing functions.
windows/window.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
commit 6aca7f1eef4ad4dd41d58424917fe6f2234b8b13
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=6aca7f1eef4ad4dd41d58424917fe6f2234b8b13;hp=322984d635c9fd8db33b60be09edd126fcfe9e54
Author: Simon Tatham <anakin at pobox.com>
Date: Sat May 27 14:59:20 2023 +0100
windows/window.c: move more variables into WinGuiSeat.
In commit f9e572595bb8cb6 I claimed that I'd removed very nearly all
the global and static variables from windows/window.c. It turns out
that this was wildly overoptimistic - I missed quite a few of them!
I'm not quite sure how I managed that; my best guess is that I used an
underpowered 'nm' command that failed to find some classes of
variable.
Some of the remaining function-scope statics were removed completely
by commit afb3dab1e97a09f just now. In this commit, I've swept up some
more and turn them into fields of WinGuiSeat, where they should have
been moved last September.
The (hopefully complete this time) list of remaining variables,
generated by running this rune in the Windows build directory:
nm windows/CMakeFiles/putty.dir/window.c.obj |
grep -E '^([^ ]+)? *[bBcCdDgGsS] [^\.]'
consists of the following variables which are legitimately global
across the whole process and not related to a particular window:
- 'hinst' and 'hprev', instance handles for Windows loadable modules
- 'classname' in the terminal_window_class_a() and
terminal_window_class_w() functions, which allocate a window class
reusably
- some pointers to Windows API functions retrieved via the
DECL_WINDOWS_FUNCTION / GET_WINDOWS_FUNCTION system, such as
p_AdjustWindowRectExForDpi and p_FlashWindowEx
- some pointers to Windows API functions set up by assigning them at
startup to the right one of the ANSI or Unicode version depending on
the Windows version, e.g. sw_DefWindowProc and sw_DispatchMessage
- 'unicode_window', a boolean flag set at the same time as those
sw_Foo function pointers
- 'sesslist', storing the last-retrieved version of the saved
sessions menu
- 'cursor_visible' in show_mouseptr() and 'forced_visible' in
update_mouse_pointer(), each of which tracks the cumulative number
of times that function has shown or hidden the mouse pointer, so as
to manage its effect on the global state updated by ShowCursor
- 'trust_icon', loaded from the executable's resources
- 'wgslisthead', the list of all active WinGuiSeats
- 'wm_mousewheel', the window-message id we use for mouse wheel
events
and the following which are nothing to do with our code:
- '_OptionsStorage' in __local_stdio_printf_options() and
__local_stdio_scanf_options(), which I'd never noticed before, but
apparently are internal to a standard library header.
windows/win-gui-seat.h | 23 ++
windows/window.c | 554 +++++++++++++++++++++++++------------------------
2 files changed, 305 insertions(+), 272 deletions(-)
More information about the tartarus-commits
mailing list