Commit
- Loading branch information
There are no files selected for viewing
12 comments
on commit 00c2cc2
There was a problem hiding this comment.
IMO using a static var here isn't really sensible, I'd say it just wastes sizeof(DocInfo) -- not that I really care, but if we're at it…
If we want to go the oh-noes-initialize-all-explicitely way, just use the verbose initializer [1] -- it's not clearer IMO that a static variable will be initialized as we want it to be than the implicitly initialized fields on a local variable being initialized as we want them to be. Otherwise, I'd be for keeping {0} and admit this isn't a problem, whatever GCC's -Wmissing-fields-initializers might tell. Or just remove -Wmissing-fields-initializers from our personal CFLAGS.
@codebrainz: setting the GCC pragma in a common header isn't a good idea, if we don't want it anywhere, just don't include it in the CFLAGS; don't start using compiler-specific thing to disable an harmless warning the merer humans won't have enabled anyway.
[1] since memset() isn't guaranteed to do what you want according to a pedant interpretation of the C standard -- NULL isn't necessarily 0, nor integer or float 0 being represented as a set of 0-filled bytes.
There was a problem hiding this comment.
There was a problem hiding this comment.
@elextr I thought I remembered you added -Wextra to HACKING already, but I don't see it, even though it has the -Wno-unused-parameter which I think comes with -Wextra and not -Wall alone. As far as doing the "proper" initialization, I dare you to write out the full initializer for this commit's particular case, I made it as far as the ... in my example :)
Anyway, I think it'd be cool to at least pick one way and stick to it for consistency, I can think of at least a few other places in Geany's code this comes up and is handled differently (than each other and this commit). If it's bad to use the guarded GCC pragmas or hardcode the flags in the build system, I say we just add the -Wno-missing-field-initializers (and -Wextra) and a note about using {0} to HACKING and use {0} in the future.
Anyone agree or disagree?
There was a problem hiding this comment.
As said on IRC, -W (which is in HACKING) is the old name for -Wextra.
@codebrainz, disagree, disabling the warning means that we don't have that protection when we write real initialisers and miss a member.
Nicks solution has minimal cost, doesn't disable warnings, and doesn't enter the "technically" incorrect area of memset.
There was a problem hiding this comment.
I agree with Lex that it's best not to disable field initializer warnings. It would be nice if gcc didn't warn for the specific case of = {0} (or if the C standard had an unambiguous 0-initializer ;-)).
Listing all initializers is a pain when there are more than about 4 IMO. I don't mind either my solution or using memset.
There was a problem hiding this comment.
I agree with Lex that it's best not to disable field initializer warnings. It would be nice if gcc didn't warn for the specific case of = {0} (or if the C standard had an unambiguous 0-initializer ;-)).
Actually, I just saw that my GCC (4.7.2) does not emit a warning for = {0} -- but properly does for = {1} :)
So I'd use = {0} initializers; but if you GCCs emit the warning, you can't live with it (I agree it can hide legitimate ones), and you can't/don't want to upgrade your GCC, I don't really mind what solution is used.
Listing all initializers is a pain when there are more than about 4 IMO.
Agreed.
There was a problem hiding this comment.
There was a problem hiding this comment.
@b4n:
GCC (4.7.2) does not emit a warning for = {0} -- but properly does for = {1}
Glad to see they fixed that. I have 4.6.1 ATM.
@codebrainz:
I think = {0} is ambiguous because the part you quoted would need the first element should be NULL, not 0. The = {0} syntax has two meanings, initialize all fields, or initialize the only integer field to zero. If it wasn't ambiguous the gcc devs wouldn't need a workaround ;-)
I prefer my fix over memset because my fix is typesafe, but if the consensus is memset I'll change it.
There was a problem hiding this comment.
@ntrel My understanding is that (irrespective of how NULL is defined/represented and irrespective of the type of the first member), this is valid and all 3 of these examples do the exact same initialization of the object's fields on all C90 and C99 compilers and all architectures everywhere (that aren't broken):
struct foo_t foo {
void *a;
int b;
float c;
};
struct foo_t bing1 = {0};
struct foo_t bing2 = {NULL};
static struct foo_t bing3;If my understanding is in fact wrong, then maybe using {0} is bad and we should use some other way. I don't really care that much either way as long as we can be consistent about it, but I think we should use the most proper and common idiom, whatever that is.
There was a problem hiding this comment.
@codebrainz (I've been offline a few days ;-)) You are right they do the same thing by the standard. The only problem comes when wanting a warning for missing initializers and the user's compiler doesn't understand {0} is a special case.
There was a problem hiding this comment.
Update: Given that newer gcc versions distinguish {0}, I've decided to use -Wno-error=missing-field-initializers until I upgrade gcc. I still see the warnings, but they don't abort my -Werror build, which is OK. (I found we're using {0} again here).
So feel free to use {0} everywhere. Shall I revert this commit now?
There was a problem hiding this comment.
I don't really mind, although I'm all for {0}. IIRC there are other locations where we used various ways of initializing, so only reverting this one maybe isn't necessary; but making them all use {0} would probably be good. So do as you want for this one, but if you would change them all to {0} I'd be happy :)
(and I plead guilty for the new use of {0}, I completely forgot about this discussion when writing that code)
This is now the 5th way I've seen to work around this annoying GCC warning message for valid C code :)
Other not as nice as just
{0}ways include:And then this seemingly better workaround was used in the past by @b4n.
Maybe we could re-add the GCC-specific one back but put it in a more central header?