Everything

php

Cleanup and maintenance of internal C code

php.internals

Andy Lester

18 years ago

Hi, I'm Andy Lester, and I'm usually a Perl person.  I've worked on
the Perl 5 and Parrot internals over the past few years, working to
clean up the C code.

The other day I was poking through the C code behind PHP 5.2 and saw
some places that I thought I could help clean up.  If the PHP core
folks are interested (and I have no idea how you PHP people handle
maintenance on things like this), I'd be glad to do some safety and
stylistic improvements that should help make the code safer.

The changes I'd be looking to make are:

* Using const qualifiers on as many core functions as possible,
* Using const qualifiers on variables to help ensure they don't get
changed accidentally
* Using variables in the smallest scope possible

I've undertaken this task in Perl 5 and Parrot to great effect, and
just had a little patch accepted here http://bugs.php.net/bug.php?id=43041
.  In the Parrot project, I wrote a guide to the benefits of consting: http://search.cpan.org/~chromatic/parrot-0.5.0/docs/project/cage_cleaners_guide.pod#Why_consting_is_good
It seems PHP is using const in some places, but many more remain to
be exploited.

Is this something you'd be interested in, or should I not bother?  If
so, how shall I begin?

thanks,
xoxo,
Andy

--
Andy Lester => andy@petdance.com => www.petdance.com => AIM:petdance

Antony Dovgal

18 years ago

On 23.11.2007 08:41, Andy Lester wrote:

> Hi, I'm Andy Lester, and I'm usually a Perl person.  I've worked on
> the Perl 5 and Parrot internals over the past few years, working to
> clean up the C code.
>
> The other day I was poking through the C code behind PHP 5.2 and saw
> some places that I thought I could help clean up.

Take a look at 5_3 and HEAD, we've done this already a couple of months ago:
http://news.php.net/php.cvs/46317

5_2 branch is for important bugfixes only, hence this improvement didn't make it there.

--
Wbr,
Antony Dovgal

Andy Lester

18 years ago

On Nov 23, 2007, at 12:19 AM, Antony Dovgal wrote:

> Take a look at 5_3 and HEAD, we've done this already a couple of
> months ago:
> http://news.php.net/php.cvs/46317

So I see.  That's a good sign.  There's still plenty of other places
to hit, too.  Are we interested in jacking up compiler warning levels,
too?  I was surprised to see that GCC doesn't default to running with -
Wall in the Makefile.

Just read through README.SUBMITTING-PATCH, so will be mailing stuff
in, as with the bug in browscap.c I just stumbled across.

xoa

--
Andy Lester => andy@petdance.com => www.petdance.com => AIM:petdance

Antony Dovgal

18 years ago

On 23.11.2007 09:25, Andy Lester wrote:

> On Nov 23, 2007, at 12:19 AM, Antony Dovgal wrote:
>
>> Take a look at 5_3 and HEAD, we've done this already a couple of
>> months ago:
>> http://news.php.net/php.cvs/46317
>
>
> So I see.  That's a good sign.  There's still plenty of other places
> to hit, too.  Are we interested in jacking up compiler warning levels,
> too?  I was surprised to see that GCC doesn't default to running with -
> Wall in the Makefile.

It does if you use --enable-debug.

--
Wbr,
Antony Dovgal

Marcus Börger

18 years ago

Hello Andy,

changing to const where possible is a damn good thing and on our list for
long. We also wanted to get rid of TSRMLS_FETCH() wherever possible. For
instance we should do this change:
typedef int (*apply_func_args_t)(void *pDest, int num_args, va_list args, zend_hash_key *hash_key);
ZEND_API void zend_hash_apply_with_arguments(HashTable *ht, apply_func_args_t apply_func, int, ...);
to:
typedef int (*apply_func_args_t)(void *pDest TSRMLS_DC, int num_args, va_list args, zend_hash_key *hash_key);
ZEND_API void zend_hash_apply_with_arguments(HashTable *ht TSRMLS_DC, apply_func_args_t apply_func, int, ...);
and there are more of those.

Since I think your offer is very good, I suggest you provide a few patches
for HEAD and 5.3 and if that turns out to be good, we'll give you cvs
access. Thats's how we all started.

marcus

Friday, November 23, 2007, 7:25:55 AM, you wrote:

> On Nov 23, 2007, at 12:19 AM, Antony Dovgal wrote:

>> Take a look at 5_3 and HEAD, we've done this already a couple of
>> months ago:
>> http://news.php.net/php.cvs/46317

> So I see.  That's a good sign.  There's still plenty of other places
> to hit, too.  Are we interested in jacking up compiler warning levels,
> too?  I was surprised to see that GCC doesn't default to running with -
> Wall in the Makefile.

> Just read through README.SUBMITTING-PATCH, so will be mailing stuff
> in, as with the bug in browscap.c I just stumbled across.

> xoa

> --
> Andy Lester => andy@petdance.com => www.petdance.com => AIM:petdance

Best regards,
Marcus