# Cleanup and maintenance of internal C code

- Thread: https://externals.io/message/33381
- Original archive: https://news-web.php.net/php.internals/33381
- Started by Andy Lester on 2007-11-23T05:41:25+00:00
- Messages: 5
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #33381 — Andy Lester — 2007-11-23T05:41:25+00:00

URL: https://externals.io/message/33381#33381

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

---

## #33382 — Antony Dovgal — 2007-11-23T06:19:27+00:00

URL: https://externals.io/message/33381#33382 · In reply to: #33381

> 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

---

## #33383 — Andy Lester — 2007-11-23T06:25:55+00:00

URL: https://externals.io/message/33381#33383 · In reply to: #33382

> 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

---

## #33384 — Antony Dovgal — 2007-11-23T06:31:23+00:00

URL: https://externals.io/message/33381#33384 · In reply to: #33383

> 
> 
> > […]
> 
> 
> 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

---

## #33388 — Marcus Boerger — 2007-11-23T10:51:11+00:00

URL: https://externals.io/message/33381#33388 · In reply to: #33383

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:

> 

> > […]


> 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