[PULL] button mapping fix and unconstify patches

[Keith Packard]

Peter Hutterer <peter.hutterer at who-t.net> writes:

> The following changes since commit 83e38eb73fd8c852513aac2da2975b4c01070ec2:
>
>   edid: Add quirk for Sony Vaio Pro 13 (2014-01-30 16:27:59 -0800)
>
> are available in the git repository at:
>
>   git://people.freedesktop.org/~whot/xserver for-keith
>
> for you to fetch changes up to 2ee2f1edc5b0954c7bf8fc4793bbb5f180a4b3fa:
>
>   dix: fix button state check before changing a button mapping (2014-02-05 07:13:03 +1000)
>
> ----------------------------------------------------------------
> Peter Hutterer (10):
>       Xi: remove superfluous cast.
>       xkb: add a call to init an XkbRMLVOSet from const chars
>       input: un-constify InputAttributes
>       Revert "os: xstrtokenize takes and returns const char * now"
>       input: un-constify dev->name
>       Revert "xfree86/parser: make strings in xf86MatchGroup const"
>       Revert "xfree86/common: handle string constants in xf86Xinput configuration"

These all look great.

To summarize the story to date -- I hacked up the X server to get rid of
almost all of the compiler warnings (I'm getting 4 at this point, but
that's with one patch that Eric is replacing), but one of those hacks
wasn't the best.

The method applied for those warning fixes was to avoid any generated
code changes; it was easy to verify that the server isn't any more
broken than it was because it's executing the same instructions. We did
the same thing when we ran the giant reformatting adventure.

Now, one thing I did at the time was to declare various 'char *'
variables 'const char *' so that you could assign constant strings or
allocated storage without needing any casts or generating any
warnings. However, this meant that a cast *was* required for the free
call.

Obviously, unless the code was changed, a cast was going to be required
some place; either when the string constant was assigned to a non-const
variable, or when the allocated storage was finally freed. I picked a
cast at 'free' time, rather than assignment for two reasons:

 1) Fewer casts. There's typically only one place the allocated storage
    would be freed, so you end up with only a single cast.

 2) Potentially catching existing mis-uses of the data. Because some
    assignments came from static strings, declaring the variables as
    const could catch existing places where the code assumed the value
    was writable.

Peter has convinced me that this plan was misguided; it's leaving a trap
for developers in the code who will be confused by this use of
'const'. And, given that I found exactly zero mis-uses of the data, it
does appear that it wasn't even as useful as I'd hoped.

Instead, what Peter has done is to switch the types back to 'char *' and
then ensure that all values assigned are not const, by strdup'ing values
as needed. This is "wasteful", but in reality, many of the places are
actually in test or configuration code, not running X server code. These
are semantic changes which do affect how the server runs, and so not
what I wanted for my exercise, but Peter argues persuasively that they
are cleaner and good for the long-term maintenance of the X server.

With this hidesight, I think now what I should have done is to cast at
the assignment of the string constant and leave the types alone. Then,
compiling with -Wcast-qual would have quickly identified all places in
the code which needed fixes like those Peter has done here. I'm willing
to change things over to this style if that seems like a useful step
towards eliminating this particular class of coding issues?

Thanks to Peter and Jasper for putting up with me on IRC this evening to
explain precisely why they didn't like the current code.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 810 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20140205/138a52eb/attachment.pgp>