scummvm/scummvm — pull request #365 by lordhoto (2013-08-03 02:31:51+00:00)
Title: RFC: Make Graphics::Surface::pixels protected.

As the title suggests this pull request is trying to make Surface::pixels protected. The motivation here is that currently direct access to "pixels" can be unsafe. For example, if you have a "const Surface &foo" you can still change the pixel data by accessing "foo.pixels" (pixels is a "void *const" in this case and not a "const void *const") directly. This does not work when you would use foo.getBasePtr. The latter seems the more intuitive behavior to me. Thus, I played a bit to remove all direct access to Surface::pixels in the code. (This is not complete yet, see below).

However, even with a protected/private "pixels" member there are two possible ways to write access pixel data.

1) const_cast away the const qualifier: This is not really an issue because it's explicit and thus we can easily see when that is absued.

2) Simply creating a copy of a const Surface. This will be harder to forbid but should hopefully be not that much of an issue in reality.

Thus, overall to me it looks like an improvement.

The following changes have been made so far:

1) Code accessing pixels has been changed to use either getBasePtr or the newly added getPixels (which is simply a getter on "pixels" and thus equivalent to getBasePtr but slightly less typing involved.)

2) Code setting pixels has been changed to use either setPixels or init. Both have been added and can be used to set pixels. "init" furthermore does setup the w, h, pitch and format.

3) Comments have been added to places which write to pixels data of const Surfaces. (Also to places which require a const_cast for other reasons like, for example, the TransparentSurface constructor in WME).

What's missing:

1) Reviewing/testing the changes.

Especially the first batch of changes to getBasePtr will need some review, since the old code didn't necessarily use the format or the pitch of the Surface it accessed pixels on. (These are commits c05cb7f3bbcf4d64d4a938e0eb42065d8f3d3038 to dbef9fef3b65cfd74a25ecb8767e1ba396f5b3b1). I made an commit for each module/engine. That way individual maintainers can review the changes more easily.

The commits 5111746911958bd35422e9614b0961a6b911ea06 to 767f0d8ca9114ea05cc1e8c5dc1940cc9718c306 have been automatically created by replacing all "getBasePtr(0, 0)" calls with "getPixels()". It would still not hurt to (again) check whether something got wrong there.

2) Adapting more backends and vkeybd.

I can do this in case we really want to make pixels private. However, I will have a hard to time to even compile test the changes.

3) Actually make "pixels" protected.

This is really easy once everything is adapated. But until then I will not do that (I did this while working on this, since it allows to find all the places which access pixels quite easily).

I'm looking forward to thoughts on this.