[WIP] GD Resources -> Objects

[markrandall]

Adding this as a PR to indicate it's being worked on.

Introduces a new class called GdImage which contains a pointer to the native gdImage resource. Still a fair amount left to do.

[markrandall]

Need to check through a lot of these tests, looks like a fair few of them deal with invalid resources, which will now be rejected by ZPP.

[markrandall]

The following tests were all based on checking for the response to an invalid resource type, mainly by opening file resources and the likes.

As resources are no longer a thing, these can all go away.

imageantialias_error1
imagechar_error2
imagecolorallocatealpha_error1
imagecolordeallocate_error1
imageellipse_error7
imagefilltoborder_error6
imagefilter_error10
imagestring_error2
imagestringup_error2
imagetruecolortopalette_error1
imagepalettetotruecolor_error3
imagelayereffect_error3
imageinterlace_error2
imagegammacorrect_error2
imagerectangle_error2
imagecharup_error2
imagecolorstotal_error
imagesetthickness_error1
imageistruecolor_error1

[krakjoe]

Also think about making the object structure private, and nit: whitespace in a couple of places

[KalleZ]

A lot of the changes seems to mix up tabs vs spaces which should be fixed too:

internal:
gd_ext_image_object
gd_ext_image_object_create
gd_ext_image_object_free
PHP_MINT_FUNCTION(gd)
_php_image_output
php_image_filter_brightness
php_image_filter_contrast
php_image_filter_colorize
php_image_filter_smooth

userland:
imagecreatetruecolor()
imagecreate()
imagecreatefromstring()
imagecrop()
imagecropauto()
imagescale()
imageaffine()

Is what I can spot with these inconsistencies during a brief look

[KalleZ]

We don't do these kind of headers for specific objects, this should be in the .c file

[markrandall]

@KalleZ The type isn't specific to the gd.c file, there's also gd_ctx.c which needs access to it.

[KalleZ]

le_gd should be removed if all the functions are converted to an object

[markrandall]

I'm reasonably happy with this now.

I've separated the object stuff out into a separate .c / .h file as I believe the gd.c file is already so long as to be overbearing and the separation will allow for object-only methods to be added in a tidy fashion in future.

I'm not going to apply the hybrid parameter parsing for the time being, that can come later.

[cmb69]

Thank you very much for working on this! I'm generally in favor to migrate ext/gd from resources to objects. And I think it's perfectly fine to only start with opaque objects (i.e. without methods).

Regarding the merging of gd_ctx.c: I think this is reasonable, but I would've preferred that as a separate PR; after all it's not particularly related to switching to objects. Now that it's integrated in this PR, that's okay.

Regarding the separate gd_image_object.(c|h): IMHO that has more drawbacks than advantages, since it's about roughly 100 additional lines to gd.c (i.e. only a few percent increase in size). If the amount of code which could go into a separate file increases, we could still refactor later on.

Regarding changing some warnings to ValueErrors: again I would have preferred to keep that separate, but well. :)

Regarding unrelated whitespace changes: I generally prefer to keep them completely separate; IMHO don't touch lines which are unrelated to a functional change. It's not a big deal in this case, though, so having the commit as part of this PR is okay.

Please have a look at gd.stub.php, which still has docblock @returns which refer to resources, but should be GdImages now.

Nit: please see https://github.com/php/php-src/blob/master/CODING_STANDARDS.md#syntax-and-indentation. I'm particularly referring to the opening brace of function definitions, which should go on its own line, and to indentation issues (seems some indentation uses spaces).

[markrandall]

Thanks for the review @cmb69 . I've pushed some changes based on your feedback. Will certainly take note of breaking things up into multiple discrete PRs for future work. I can revert the whitespace changes to the other parts of the file if it would help wrap things up.

Could you explain what you mean by drawbacks to the separate files? I saw that some extensions were using them (curl, opcache, pdo, previous gd etc) and some were not (reflection, posix, xmlwriter) but wasn't aware the choice could have negatives associated with it.

[cmb69]

Re whitespace changes: I still think it's okay to keep them, now they're done. :)

Re separate files drawbacks: I was referring to complexity and optimization (optimizability, if that's a word). The complexity increases in this case, since there are two modules which are tightly coupled, so it's hard to understand one without knowing the other. Actually, I don't think that these should be separate modules at all. And the compiler alone can likely optimize better if there's only a single module (well, if link time program optimization is supported, that may not matter). So yes, not much drawbacks, but I still think that this slightly outweights the advantages.

[nikic]

Side note: Due to the use of global registers, PHP is incompatible with LTO when using GCC. So it's generally a good assumption that no cross compile unit optimization occurs.

[markrandall]

Merged everything into one file, moved the contents of the former gd_ctx.c to the bottom, the object stuff to the top, and the guts of the GD extension to the middle, and threw in some comment blocks to create a more obvious separation when scrolling.

[cmb69]

Thank you very much! Applied as 8aad466.