Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [PATCH] Clean up struct initalizations »

[PATCH] Clean up struct initalizations

php.internals

Andy Lester

18 years ago
I've been working on making PHP build under more stringent error checking, specifically -Wextra under GCC. This patch cleans up dozens of struct initializations that are valid C, but could hide future problems. xoxo, Andy
-- Andy Lester => andy@petdance.com => www.petdance.com => AIM:petdance

Stanislav Malyshev

18 years ago
> I've been working on making PHP build under more stringent error > checking, specifically -Wextra under GCC. This patch cleans up dozens > of struct initializations that are valid C, but could hide future > problems.
Doesn't C standard mandate filling up uninitialized fields with 0?
-- Stanislav Malyshev, Zend Software Architect stas@zend.com http://www.zend.com/ (408)253-8829 MSN: stas@zend.com

Marcus Börger

18 years ago
Hello Stanislav, a global struct is filled up with NULL/0's while a local struct is not initialized at all. The patch simply provides more correct code and even adds a few consts. I think we should apply this to 5.3 and HEAD. Meaning we need it for both versions. marcus Tuesday, November 27, 2007, 9:53:12 AM, you wrote:
>> I've been working on making PHP build under more stringent error >> checking, specifically -Wextra under GCC. This patch cleans up dozens >> of struct initializations that are valid C, but could hide future >> problems.
> Doesn't C standard mandate filling up uninitialized fields with 0? > -- > Stanislav Malyshev, Zend Software Architect > stas@zend.com http://www.zend.com/ > (408)253-8829 MSN: stas@zend.com
Best regards, Marcus

Stanislav Malyshev

18 years ago
> a global struct is filled up with NULL/0's while a local struct is not > initialized at all. The patch simply provides more correct code and even
That's not what I read in C standard - it says that if struct is partially initialized, the rest is zeroed out: Quoting http://c0x.coding-guidelines.com/6.7.8.html: 1671 If there are fewer initializers in a brace-enclosed list than there are elements or members of an aggregate, or fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration. 1642 If an object that has static storage duration is not initialized explicitly, then: 1643 — if it has pointer type, it is initialized to a null pointer; 1644 — if it has arithmetic type, it is initialized to (positive or unsigned) zero; 1645 — if it is an aggregate, every member is initialized (recursively) according to these rules; 1646 — if it is a union, the first named member is initialized (recursively) according to these rule All compilers I know do behave this way. Do not confuse partial initialization wiht no initialization at all, of course.
-- Stanislav Malyshev, Zend Software Architect stas@zend.com http://www.zend.com/ (408)253-8829 MSN: stas@zend.com

Alexey Zakhlestin

18 years ago
you are quoting c99… and we are still using "c90" mode, as far as I remember On 11/27/07, Stanislav Malyshev <stas@zend.com> wrote: > > a global struct is filled up with NULL/0's while a local struct is not > > initialized at all. The patch simply provides more correct code and even > > That's not what I read in C standard - it says that if struct is > partially initialized, the rest is zeroed out: > > Quoting http://c0x.coding-guidelines.com/6.7.8.html: > > 1671 If there are fewer initializers in a brace-enclosed list than there > are elements or members of an aggregate, or fewer characters in a string > literal used to initialize an array of known size than there are > elements in the array, the remainder of the aggregate shall be > initialized implicitly the same as objects that have static storage > duration. > > 1642 If an object that has static storage duration is not initialized > explicitly, then: > 1643 — if it has pointer type, it is initialized to a null pointer; > 1644 — if it has arithmetic type, it is initialized to (positive or > unsigned) zero; > 1645 — if it is an aggregate, every member is initialized (recursively) > according to these rules; > 1646 — if it is a union, the first named member is initialized > (recursively) according to these rule > > All compilers I know do behave this way. Do not confuse partial > initialization wiht no initialization at all, of course. > -- > Stanislav Malyshev, Zend Software Architect > stas@zend.com http://www.zend.com/ > (408)253-8829 MSN: stas@zend.com > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > > -- Alexey Zakhlestin http://blog.milkfarmsoft.com/

Stanislav Malyshev

18 years ago
> you are quoting c99… > and we are still using "c90" mode, as far as I remember
I couldn't find any c90 available online for free but anyway that's long-standing practice that all compilers I know follow. gcc's own manual says: -Wmissing-field-initializers Warn if a structure's initializer has some fields missing. For example, the following code would cause such a warning, because x.h is implicitly zero: struct s { int f, g, h; }; struct s x = { 3, 4 };
-- Stanislav Malyshev, Zend Software Architect stas@zend.com http://www.zend.com/ (408)253-8829 MSN: stas@zend.com

Andy Lester

18 years ago
On Nov 27, 2007, at 2:53 AM, Stanislav Malyshev wrote:
>> I've been working on making PHP build under more stringent error >> checking, specifically -Wextra under GCC. This patch cleans up >> dozens >> of struct initializations that are valid C, but could hide future >> problems. > > Doesn't C standard mandate filling up uninitialized fields with 0?
Yes. My patch is for human benefit, not the compiler's.
-- Andy Lester => andy@petdance.com => www.petdance.com => AIM:petdance

Ilia A.

18 years ago
IMHO {0} is sufficiently clear for human consumption. On 27-Nov-07, at 8:49 AM, Andy Lester wrote:
> > On Nov 27, 2007, at 2:53 AM, Stanislav Malyshev wrote: > >>> I've been working on making PHP build under more stringent error >>> checking, specifically -Wextra under GCC. This patch cleans up >>> dozens >>> of struct initializations that are valid C, but could hide future >>> problems. >> >> Doesn't C standard mandate filling up uninitialized fields with 0? > > > Yes. My patch is for human benefit, not the compiler's. > > -- > Andy Lester => andy@petdance.com => www.petdance.com => AIM:petdance > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php >
Ilia Alshanetsky

Alexey Zakhlestin

18 years ago
On 11/27/07, Ilia Alshanetsky <ilia@prohost.org> wrote:
> IMHO {0} is sufficiently clear for human consumption.
{NULL, 0, 0} brings real structure dimensions to the context, which is good (less need to check "headers").
> On 27-Nov-07, at 8:49 AM, Andy Lester wrote: > > > > > On Nov 27, 2007, at 2:53 AM, Stanislav Malyshev wrote: > > > >>> I've been working on making PHP build under more stringent error > >>> checking, specifically -Wextra under GCC. This patch cleans up > >>> dozens > >>> of struct initializations that are valid C, but could hide future > >>> problems. > >> > >> Doesn't C standard mandate filling up uninitialized fields with 0? > > > > > > Yes. My patch is for human benefit, not the compiler's. > > > > -- > > Andy Lester => andy@petdance.com => www.petdance.com => AIM:petdance > > > > -- > > PHP Internals - PHP Runtime Development Mailing List > > To unsubscribe, visit: http://www.php.net/unsub.php > > > > Ilia Alshanetsky > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > >
-- Alexey Zakhlestin http://blog.milkfarmsoft.com/

Andy Lester

18 years ago
On Nov 27, 2007, at 8:34 AM, Alexey Zakhlestin wrote:
> On 11/27/07, Ilia Alshanetsky <ilia@prohost.org> wrote: >> IMHO {0} is sufficiently clear for human consumption. > > {NULL, 0, 0} brings real structure dimensions to the context, which is > good (less need to check "headers").
It also means that you don't get compiler errors any more like you would with {0}. Now, you can know that if there's a warning about the number of initializers that it's something you should pay attention to. The key for long-term maintenance is to clean up the slop so that the compiler doesn't squawk about it at high levels of warnings. Then when you do have real squawk-worthy problems, they don't get lost in the noise of meaningless warnings.
-- Andy Lester => andy@petdance.com => www.petdance.com => AIM:petdance

Ilia A.

18 years ago
GCC will spue a slew of warnings that can be safely ignored and in some cases are bogus. The pedantic changes only make the code harder to read and yield very little if any benefits in return. For larger, more complex structs like zvals it'll only create a meaningless mess. There is absolutely no reason that any half decent compiler will not be able to understand {0} for any declared struct. On 27-Nov-07, at 10:12 AM, Andy Lester wrote:
> > On Nov 27, 2007, at 8:34 AM, Alexey Zakhlestin wrote: > >> On 11/27/07, Ilia Alshanetsky <ilia@prohost.org> wrote: >>> IMHO {0} is sufficiently clear for human consumption. >> >> {NULL, 0, 0} brings real structure dimensions to the context, which >> is >> good (less need to check "headers"). > > It also means that you don't get compiler errors any more like you > would with {0}. Now, you can know that if there's a warning about > the number of initializers that it's something you should pay > attention to. > > The key for long-term maintenance is to clean up the slop so that > the compiler doesn't squawk about it at high levels of warnings. > Then when you do have real squawk-worthy problems, they don't get > lost in the noise of meaningless warnings. > > > -- > Andy Lester => andy@petdance.com => www.petdance.com => AIM:petdance > > > >
Ilia Alshanetsky

Andy Lester

18 years ago
On Nov 27, 2007, at 9:49 AM, Ilia Alshanetsky wrote:
> GCC will spue a slew of warnings that can be safely ignored and in > some cases are bogus. The pedantic changes only make the code harder > to read and yield very little if any benefits in return. For larger, > more complex structs like zvals it'll only create a meaningless > mess. There is absolutely no reason that any half decent compiler > will not be able to understand {0} for any declared struct.
My goal here is to improve long-term maintainability of the code, by letting compilers and tools like splint do their automated magic where possible. If this is not useful to the PHP core team, then that's fine, I'll walk away. Let me know either way. xoa
-- Andy Lester => andy@petdance.com => www.petdance.com => AIM:petdance

Ilia A.

18 years ago
Andy, There is absolutely nothing wrong with trying to make the code more maintainable, in fact I think everyone, myself included welcome any work in that direction. But at the same time we need to keep in mind that compiler is not always right and often will generate meaningless warnings even when there is absolutely nothing wrong the code. Changes for the sake of making the compiler happy with -Winsane-warnings ;-) does not really help maintainability. In the case of {0} vs {NULL, 0,0}, my opinion is that the former is far more readable and understandable. On 27-Nov-07, at 10:50 AM, Andy Lester wrote:
> > On Nov 27, 2007, at 9:49 AM, Ilia Alshanetsky wrote: > >> GCC will spue a slew of warnings that can be safely ignored and in >> some cases are bogus. The pedantic changes only make the code >> harder to read and yield very little if any benefits in return. For >> larger, more complex structs like zvals it'll only create a >> meaningless mess. There is absolutely no reason that any half >> decent compiler will not be able to understand {0} for any declared >> struct. > > > My goal here is to improve long-term maintainability of the code, by > letting compilers and tools like splint do their automated magic > where possible. If this is not useful to the PHP core team, then > that's fine, I'll walk away. Let me know either way. > > xoa > > -- > Andy Lester => andy@petdance.com => www.petdance.com => AIM:petdance > > > >
Ilia Alshanetsky

Andy Lester

18 years ago
On Nov 27, 2007, at 9:57 AM, Ilia Alshanetsky wrote:
> There is absolutely nothing wrong with trying to make the code more > maintainable, in fact I think everyone, myself included welcome any > work in that direction. But at the same time we need to keep in mind > that compiler is not always right and often will generate > meaningless warnings even when there is absolutely nothing wrong the > code. Changes for the sake of making the compiler happy with - > Winsane-warnings ;-) does not really help maintainability. In the > case of {0} vs {NULL,0,0}, my opinion is that the former is far more > readable and understandable.
I'm trying to look at the code with the eyes of a human, trying to use the compiler as my ally, not something to be circumvented. GCC has most of the features of a good lint, if we take advantage of it. Think of it as compiler-enforced coding standards. My goal has been to try to raise up the coding level of the code that I see, that I'm using in my day-to-day work. The more seat belts the better, and initializing structs is just one of them. Adding consts and fixing signed/unsigned mismatches are on my list, too, but there's just so much that's slop that I'm trying to get the low-hanging fruit early. xoa
-- Andy Lester => andy@petdance.com => www.petdance.com => AIM:petdance

Stanislav Malyshev

18 years ago
> My goal here is to improve long-term maintainability of the code, by
I don't see how unwrapping all structures in initializers improves maintainability. If anything, it actually makes code harder to maintain - now on any change you need to go over all the code and fix all the initializers - a ton of completely meaningless work since a compiler can do it for you and the standard specifically provides means to do it. Now if there were non-zero initializers, that would be different, but with zero inits I don't see any reason not to leave them be.
-- Stanislav Malyshev, Zend Software Architect stas@zend.com http://www.zend.com/ (408)253-8829 MSN: stas@zend.com

Stanislav Malyshev

18 years ago
>> Doesn't C standard mandate filling up uninitialized fields with 0? > > > Yes. My patch is for human benefit, not the compiler's.
Not sure I see how it helps - suppose we add a field into structure, does it mean one has to go over all the code that ever mentioned this structure and add zeroes? How helpful is that?
-- Stanislav Malyshev, Zend Software Architect stas@zend.com http://www.zend.com/ (408)253-8829 MSN: stas@zend.com

Andy Lester

18 years ago
> Not sure I see how it helps - suppose we add a field into structure, > does it mean one has to go over all the code that ever mentioned this > structure and add zeroes? How helpful is that?
Makes sure you got them all, doesn't it? xoa
-- Andy Lester => andy@petdance.com => www.petdance.com => AIM:petdance

Stanislav Malyshev

18 years ago
>> Not sure I see how it helps - suppose we add a field into structure, >> does it mean one has to go over all the code that ever mentioned this >> structure and add zeroes? How helpful is that? > > Makes sure you got them all, doesn't it?
No, the compiler makes sure I got them all, and it does it with {0}.
-- Stanislav Malyshev, Zend Software Architect stas@zend.com http://www.zend.com/ (408)253-8829 MSN: stas@zend.com

Andy Lester

18 years ago
On Tue, Nov 27, 2007 at 10:30:35AM -0800, Stanislav Malyshev (stas@zend.com) wrote:
> No, the compiler makes sure I got them all, and it does it with {0}.
I can tell where this is going. I'll back away now. xoa
-- Andy Lester => andy@petdance.com => www.petdance.com => AIM:petdance