The values of that field in a Control structure are already
platform-dependent: you're only supposed to set them in cross-platform
code by using #defined names that each platform will define
differently.
Now I need the _type_ as well as the values to be opaque, because I'm
about to make a change on Windows that turns it into a wide character
string instead of a char string.
I mentioned recently (in commit 9e7d4c53d8) message that I'm no
longer fond of the variable name 'ret', because it's used in two quite
different contexts: it's the return value from a subroutine you just
called (e.g. 'int ret = read(fd, buf, len);' and then check for error
or EOF), or it's the value you're preparing to return from the
_containing_ routine (maybe by assigning it a default value and then
conditionally modifying it, or by starting at NULL and reallocating,
or setting it just before using the 'goto out' cleanup idiom). In the
past I've occasionally made mistakes by forgetting which meaning the
variable had, or accidentally conflating both uses.
If all else fails, I now prefer 'retd' (short for 'returned') in the
former situation, and 'toret' (obviously, the value 'to return') in
the latter case. But even better is to pick a name that actually says
something more specific about what the thing actually is.
One particular bad habit throughout this codebase is to have a set of
functions that deal with some object type (say 'Foo'), all *but one*
of which take a 'Foo *foo' parameter, but the foo_new() function
starts with 'Foo *ret = snew(Foo)'. If all the rest of them think the
canonical name for the ambient Foo is 'foo', so should foo_new()!
So here's a no-brainer start on cutting down on the uses of 'ret': I
looked for all the cases where it was being assigned the result of an
allocation, and renamed the variable to be a description of the thing
being allocated. In the case of a new() function belonging to a
family, I picked the same name as the rest of the functions in its own
family, for consistency. In other cases I picked something sensible.
One case where it _does_ make sense not to use your usual name for the
variable type is when you're cloning an existing object. In that case,
_neither_ of the Foo objects involved should be called 'foo', because
it's ambiguous! They should be named so you can see which is which. In
the two cases I found here, I've called them 'orig' and 'copy'.
As in the previous refactoring, many thanks to clang-rename for the
help.
In commit 694d5184b7, I introduced the 'just_button' flag for
CTRL_FILESELECT controls, and in commit ddcd93ab12 I added a use
of such a control with the flag set to true.
But I forgot to set it to false everywhere else, which caused an
assertion failure when selecting the Bell pane in Windows PuTTY. Oops.
Partly, this just seems more sensible, since it may well vary per
platform beyond the ability of intorptr to specify. But more
immediately it means the definition of the HELPCTX macro doesn't have
to use the P() function from dialog.h, which isn't defined in any
circumstances outside the config subsystem. And I'm about to want to
put a help context well outside that subsystem.
ctrl_radiobuttons has a variadic argument list that it expects to be
terminated by a null pointer where a const char * should be. So the
terminating NULL at each call site ought to be cast to const char *,
for the usual reason (NULL might expand literally to something not the
same size as a pointer and get mis-marshalled in the variadic argument
list). It wasn't being.
Fixed in the same way as dupcat: I've turned ctrl_radiobuttons into a
macro wrapper on the underlying function, which adds the NULL
automatically with its correct cast. This also saves verbiage at every
call site, because now I can leave out all the NULLs there.
This is for cases where they're presenting information to the user
that wouldn't wrap sensibly anyway (such as an SSH key fingerprint
which is mostly all one word), and in which newlines might be
significant.
On GTK, the implementing widget is still a GtkLabel, but without the
wrap flag set, and wrapped in a GtkScrolledWindow in case the text is
too wide to fit.
On Windows, I've switched to using an edit box instead of a static
text control, making it readonly, and borderless via my existing
MakeDlgItemBorderless helper function. This doesn't get you an actual
scrollbar, but it does mean you can scroll left and right by dragging
with the mouse.
This gets rid of that awkward STANDARD_PREFIX system in which every
branch of the old 'union control' had to repeat all the generic
fields, and then call sites had to make an arbitrary decision about
which branch to access them through.
That was the best we could do before accepting C99 features in this
code base. But now we have anonymous unions, so we don't need to put
up with that nonsense any more!
'dlgcontrol' is now a struct rather than a union, and the generic
fields common to all control types are ordinary members of the struct,
so you don't have to refer to them as ctrl->generic.foo at all, just
ctrl->foo, which saves verbiage at the point of use.
The extra per-control fields are still held in structures named after
the control type, so you'll still say ctrl->listbox.height or
whatever. But now those structures are themselves members of an
anonymous union field following the generic fields, so those
sub-structures don't have to reiterate all the standard stuff too.
While I'm here, I've promoted 'context2' from an editbox-specific
field to a generic one (it just seems silly _not_ to allow any control
to have two context fields if it needs it). Also, I had to rename the
boolean field 'tabdelay' to avoid it clashing with the subsidiary
structure field 'tabdelay', now that the former isn't generic.tabdelay
any more.
I'm about to change my mind about whether its top-level nature is
struct or union, and rather than change the key word 'union' to
'struct' at every point of use, it's nicer to just get rid of the
keyword completely. So it has a shiny new name.
This will let us put two controls side by side (e.g. in disjoint
columns of a multi-col layout) and indicate that instead of the
default behaviour of aligning their top edges, their centreline (or,
even better if available, font baseline) should be aligned.
NFC: nothing uses this yet.
The number of people has been steadily increasing who read our source
code with an editor that thinks tab stops are 4 spaces apart, as
opposed to the traditional tty-derived 8 that the PuTTY code expects.
So I've been wondering for ages about just fixing it, and switching to
a spaces-only policy throughout the code. And I recently found out
about 'git blame -w', which should make this change not too disruptive
for the purposes of source-control archaeology; so perhaps now is the
time.
While I'm at it, I've also taken the opportunity to remove all the
trailing spaces from source lines (on the basis that git dislikes
them, and is the only thing that seems to have a strong opinion one
way or the other).
Apologies to anyone downstream of this code who has complicated patch
sets to rebase past this change. I don't intend it to be needed again.
The idea of these is that they centralise the common idiom along the
lines of
if (logical_array_len >= physical_array_size) {
physical_array_size = logical_array_len * 5 / 4 + 256;
array = sresize(array, physical_array_size, ElementType);
}
which happens at a zillion call sites throughout this code base, with
different random choices of the geometric factor and additive
constant, sometimes forgetting them completely, and generally doing a
lot of repeated work.
The new macro sgrowarray(array,size,n) has the semantics: here are the
array pointer and its physical size for you to modify, now please
ensure that the nth element exists, so I can write into it. And
sgrowarrayn(array,size,n,m) is the same except that it ensures that
the array has size at least n+m (so sgrowarray is just the special
case where m=1).
Now that this is a single centralised implementation that will be used
everywhere, I've also gone to more effort in the implementation, with
careful overflow checks that would have been painful to put at all the
previous call sites.
This commit also switches over every use of sresize(), apart from a
few where I really didn't think it would gain anything. A consequence
of that is that a lot of array-size variables have to have their types
changed to size_t, because the macros require that (they address-take
the size to pass to the underlying function).
My normal habit these days, in new code, is to treat int and bool as
_almost_ completely separate types. I'm still willing to use C's
implicit test for zero on an integer (e.g. 'if (!blob.len)' is fine,
no need to spell it out as blob.len != 0), but generally, if a
variable is going to be conceptually a boolean, I like to declare it
bool and assign to it using 'true' or 'false' rather than 0 or 1.
PuTTY is an exception, because it predates the C99 bool, and I've
stuck to its existing coding style even when adding new code to it.
But it's been annoying me more and more, so now that I've decided C99
bool is an acceptable thing to require from our toolchain in the first
place, here's a quite thorough trawl through the source doing
'boolification'. Many variables and function parameters are now typed
as bool rather than int; many assignments of 0 or 1 to those variables
are now spelled 'true' or 'false'.
I managed this thorough conversion with the help of a custom clang
plugin that I wrote to trawl the AST and apply heuristics to point out
where things might want changing. So I've even managed to do a decent
job on parts of the code I haven't looked at in years!
To make the plugin's work easier, I pushed platform front ends
generally in the direction of using standard 'bool' in preference to
platform-specific boolean types like Windows BOOL or GTK's gboolean;
I've left the platform booleans in places they _have_ to be for the
platform APIs to work right, but variables only used by my own code
have been converted wherever I found them.
In a few places there are int values that look very like booleans in
_most_ of the places they're used, but have a rarely-used third value,
or a distinction between different nonzero values that most users
don't care about. In these cases, I've _removed_ uses of 'true' and
'false' for the return values, to emphasise that there's something
more subtle going on than a simple boolean answer:
- the 'multisel' field in dialog.h's list box structure, for which
the GTK front end in particular recognises a difference between 1
and 2 but nearly everything else treats as boolean
- the 'urgent' parameter to plug_receive, where 1 vs 2 tells you
something about the specific location of the urgent pointer, but
most clients only care about 0 vs 'something nonzero'
- the return value of wc_match, where -1 indicates a syntax error in
the wildcard.
- the return values from SSH-1 RSA-key loading functions, which use
-1 for 'wrong passphrase' and 0 for all other failures (so any
caller which already knows it's not loading an _encrypted private_
key can treat them as boolean)
- term->esc_query, and the 'query' parameter in toggle_mode in
terminal.c, which _usually_ hold 0 for ESC[123h or 1 for ESC[?123h,
but can also hold -1 for some other intervening character that we
don't support.
In a few places there's an integer that I haven't turned into a bool
even though it really _can_ only take values 0 or 1 (and, as above,
tried to make the call sites consistent in not calling those values
true and false), on the grounds that I thought it would make it more
confusing to imply that the 0 value was in some sense 'negative' or
bad and the 1 positive or good:
- the return value of plug_accepting uses the POSIXish convention of
0=success and nonzero=error; I think if I made it bool then I'd
also want to reverse its sense, and that's a job for a separate
piece of work.
- the 'screen' parameter to lineptr() in terminal.c, where 0 and 1
represent the default and alternate screens. There's no obvious
reason why one of those should be considered 'true' or 'positive'
or 'success' - they're just indices - so I've left it as int.
ssh_scp_recv had particularly confusing semantics for its previous int
return value: its call sites used '<= 0' to check for error, but it
never actually returned a negative number, just 0 or 1. Now the
function and its call sites agree that it's a bool.
In a couple of places I've renamed variables called 'ret', because I
don't like that name any more - it's unclear whether it means the
return value (in preparation) for the _containing_ function or the
return value received from a subroutine call, and occasionally I've
accidentally used the same variable for both and introduced a bug. So
where one of those got in my way, I've renamed it to 'toret' or 'retd'
(the latter short for 'returned') in line with my usual modern
practice, but I haven't done a thorough job of finding all of them.
Finally, one amusing side effect of doing this is that I've had to
separate quite a few chained assignments. It used to be perfectly fine
to write 'a = b = c = TRUE' when a,b,c were int and TRUE was just a
the 'true' defined by stdbool.h, that idiom provokes a warning from
gcc: 'suggest parentheses around assignment used as truth value'!
This commit includes <stdbool.h> from defs.h and deletes my
traditional definitions of TRUE and FALSE, but other than that, it's a
100% mechanical search-and-replace transforming all uses of TRUE and
FALSE into the C99-standardised lowercase spellings.
No actual types are changed in this commit; that will come next. This
is just getting the noise out of the way, so that subsequent commits
can have a higher proportion of signal.
Having found a lot of unfixed constness issues in recent development,
I thought perhaps it was time to get proactive, so I compiled the
whole codebase with -Wwrite-strings. That turned up a huge load of
const problems, which I've fixed in this commit: the Unix build now
goes cleanly through with -Wwrite-strings, and the Windows build is as
close as I could get it (there are some lingering issues due to
occasional Windows API functions like AcquireCredentialsHandle not
having the right constness).
Notable fallout beyond the purely mechanical changing of types:
- the stuff saved by cmdline_save_param() is now explicitly
dupstr()ed, and freed in cmdline_run_saved.
- I couldn't make both string arguments to cmdline_process_param()
const, because it intentionally writes to one of them in the case
where it's the argument to -pw (in the vain hope of being at least
slightly friendly to 'ps'), so elsewhere I had to temporarily
dupstr() something for the sake of passing it to that function
- I had to invent a silly parallel version of const_cmp() so I could
pass const string literals in to lookup functions.
- stripslashes() in pscp.c and psftp.c has the annoying strchr nature
I'm about to add a list box which expects to contain some very long
but uninformative strings, and which is also quite vertically squashed
so there's not much room for a horizontal scroll bar to appear in it.
So here's an option in the list box specification structure which
causes the constructed GTKTreeView to use the 'ellipsize' option for
all its cell renderers, i.e. too-long strings are truncated with an
ellipsis.
Windows needs no change, because its list boxes already work this way.
[originally from svn r10219]
custom free function, in case you need to ctrl_alloc a structure which
then has additional dynamically allocated things dangling off it.
[originally from svn r9922]
'Config' in putty.h, which stores all PuTTY's settings and includes an
arbitrary length limit on every single one of those settings which is
stored in string form. In place of it is 'Conf', an opaque data type
everywhere outside the new file conf.c, which stores a list of (key,
value) pairs in which every key contains an integer identifying a
configuration setting, and for some of those integers the key also
contains extra parts (so that, for instance, CONF_environmt is a
string-to-string mapping). Everywhere that a Config was previously
used, a Conf is now; everywhere there was a Config structure copy,
conf_copy() is called; every lookup, adjustment, load and save
operation on a Config has been rewritten; and there's a mechanism for
serialising a Conf into a binary blob and back for use with Duplicate
Session.
User-visible effects of this change _should_ be minimal, though I
don't doubt I've introduced one or two bugs here and there which will
eventually be found. The _intended_ visible effects of this change are
that all arbitrary limits on configuration strings and lists (e.g.
limit on number of port forwardings) should now disappear; that list
boxes in the configuration will now be displayed in a sorted order
rather than the arbitrary order in which they were added to the list
(since the underlying data structure is now a sorted tree234 rather
than an ad-hoc comma-separated string); and one more specific change,
which is that local and dynamic port forwardings on the same port
number are now mutually exclusive in the configuration (putting 'D' in
the key rather than the value was a mistake in the first place).
One other reorganisation as a result of this is that I've moved all
the dialog.c standard handlers (dlg_stdeditbox_handler and friends)
out into config.c, because I can't really justify calling them generic
any more. When they took a pointer to an arbitrary structure type and
the offset of a field within that structure, they were independent of
whether that structure was a Config or something completely different,
but now they really do expect to talk to a Conf, which can _only_ be
used for PuTTY configuration, so I've renamed them all things like
conf_editbox_handler and moved them out of the nominally independent
dialog-box management module into the PuTTY-specific config.c.
[originally from svn r9214]
malloc functions, which automatically cast to the same type they're
allocating the size of. Should prevent any future errors involving
mallocing the size of the wrong structure type, and will also make
life easier if we ever need to turn the PuTTY core code from real C
into C++-friendly C. I haven't touched the Mac frontend in this
checkin because I couldn't compile or test it.
[originally from svn r3014]
Buttons now have an `iscancel' flag to go with `isdefault';
dlg_last_focused() now explicitly passes the control it _doesn't_
care about (`I want the last control that had focus and isn't this
one'); and in the GTK implementation, various fixes have happened,
notably including arrow keys working sensibly in list boxes and the
treeview and short font aliases being expanded correctly to
initialise the font selectors.
[originally from svn r2958]
to pieces, and put it back together in a new table-driven form.
config.c sets up a data structure describing most of the config box;
wincfg.c adds in the Windows-specific options (so that config.c can
also form the basis for Mac and Unix config boxes). Then winctrls.c
contains a shiny new layout engine which consumes that data
structure, and windlg.c passes all WM_COMMAND and similar messages
to a driver alongside that layout engine. In the process I've sorted
out nicer-looking panel titles and finally fixed the list-boxes-are-
never-the-right-size bug (turned out to be Windows's fault, of
course). I _believe_ it should do everything the old config box did,
including context help. Now everyone has to test it thoroughly...
[originally from svn r2908]
