You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Matthew Woehlke <mw...@users.sourceforge.net> on 2009/03/25 23:05:56 UTC
[patch] c99-isms
I'm trying to build svn 1.6.0 (and previously 1.5.6, which had the same
problems) on one-and-a-half* machines that lack a c99 compiler. This
causes problems because svn uses a c99 convention of initializing
structs with "non-static" data.
The attached patch is needed for compilation to succeed on these
platforms. Generally speaking, these problems are:
- struct initialization with "non-const" data
- comma after last item in an enum
- initialize function pointers to NULL (use '0' instead)
(* The "half" is AIX, which has a c99-capable compiler, but autoconf is
cleverly breaking it by picking -qlanglvl=ansi to get c89-compliance, at
which point it fails to find the -qlanglvl option to turn on c99 support.)
--
Matthew
Please do not quote my e-mail address unobfuscated in message bodies.
--
"Yoda of Borg am I. Futile is resistance. Assimilate you, I will."
-- from http://en.wikipedia.org/wiki/Wikipedia:Yet_more_Best_of_BJAODN
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1421373
Re: [patch] c99-isms
Posted by "C. Michael Pilato" <cm...@collab.net>.
Matthew Woehlke wrote:
> I'm trying to build svn 1.6.0 (and previously 1.5.6, which had the same
> problems) on one-and-a-half* machines that lack a c99 compiler. This
> causes problems because svn uses a c99 convention of initializing
> structs with "non-static" data.
>
> The attached patch is needed for compilation to succeed on these
> platforms. Generally speaking, these problems are:
> - struct initialization with "non-const" data
> - comma after last item in an enum
> - initialize function pointers to NULL (use '0' instead)
>
> (* The "half" is AIX, which has a c99-capable compiler, but autoconf is
> cleverly breaking it by picking -qlanglvl=ansi to get c89-compliance, at
> which point it fails to find the -qlanglvl option to turn on c99 support.)
Thanks, Matthew. I'm looking at your patch now.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1431823
Re: [patch] c99-isms
Posted by Branko Cibej <br...@xbc.nu>.
C. Michael Pilato wrote:
> Branko Čibej wrote:
>
>> Also ... you actually *can* init almost all of argv here. :)
>>
>> const char *argv[] = { "opt-test", NULL, NULL };
>>
>> and later:
>>
>> argv[1] = input;
>>
>
> I find that kinda confusing, though (as a reader of the code). Don't you?
>
No. But spell my name. :-P
-- Brane
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1444622
Re: [patch] c99-isms
Posted by "C. Michael Pilato" <cm...@collab.net>.
Branko Čibej wrote:
> C. Michael Pilato wrote:
>> Matthew Woehlke wrote:
>>
>>> I'm trying to build svn 1.6.0 (and previously 1.5.6, which had the same
>>> problems) on one-and-a-half* machines that lack a c99 compiler. This
>>> causes problems because svn uses a c99 convention of initializing
>>> structs with "non-static" data.
>>>
>>> The attached patch is needed for compilation to succeed on these
>>> platforms. Generally speaking, these problems are:
>>> - struct initialization with "non-const" data
>>> - comma after last item in an enum
>>> - initialize function pointers to NULL (use '0' instead)
>>>
>> Now, you do realize that we only aspire to C89/ANSI-C, right? This is
>> documented in our www/hacking.html file. So, some of what you recommend is
>> fine because it satisfies both C89 and C99. But some of it, methinks, won't
>> pan out well for C89 compilers, namely stuff like this:
>>
>>
>>> diff -ru subversion-1.6.0/subversion/tests/libsvn_client/client-test.c subversion-1.6.0-patched/subversion/tests/libsvn_client/client-test.c
>>> --- subversion-1.6.0/subversion/tests/libsvn_client/client-test.c 2008-07-30 08:05:17.000000000 -0700
>>> +++ subversion-1.6.0-patched/subversion/tests/libsvn_client/client-test.c 2009-03-25 14:57:04.536775000 -0700
>>> @@ -146,7 +146,10 @@
>>> apr_array_header_t *targets;
>>> apr_getopt_t *os;
>>> const int argc = 2;
>>> - const char *argv[] = { "opt-test", input, NULL };
>>> + const char *argv[3];
>>> + argv[0] = "opt-test";
>>> + argv[1] = input;
>>> + argv[2] = NULL;
>>> apr_status_t apr_err;
>>> svn_error_t *err;
>>>
>> Cases like this will (I'm guessing) cause problems because they declare
>> stack variables after other assignments have been made. We need to move
>> those "argv[N] =" lines to after all the variable declarations. Wanna take
>> another cut at the patch?
>>
>
> Also ... you actually *can* init almost all of argv here. :)
>
> const char *argv[] = { "opt-test", NULL, NULL };
>
> and later:
>
> argv[1] = input;
I find that kinda confusing, though (as a reader of the code). Don't you?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1432522
Re: [patch] c99-isms
Posted by Matthew Woehlke <mw...@users.sourceforge.net>.
Branko Cibej wrote:
> Also ... you actually *can* init almost all of argv here. :)
>
> const char *argv[] = { "opt-test", NULL, NULL };
>
> and later:
>
> argv[1] = input;
Yes, it's a stylistic decision. Filling a list that is effectively a
bunch of pointers plus a NULL to mark the end, it feels a bit strange to
me to do some constant initialization and then "inject" things, versus
an explicit assignment to terminate it. But as long as it compiles, I
really don't care what style you prefer :-). I just picked one and went
with it.
--
Matthew
Please do not quote my e-mail address unobfuscated in message bodies.
--
"We're all mad here. I'm mad. You're mad... You must be, or you wouldn't
have come here." -- The Cheshire Cat (from Lewis Carroll's Alice in
Wonderland)
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1432547
Re: [patch] c99-isms
Posted by Branko Cibej <br...@xbc.nu>.
C. Michael Pilato wrote:
> Matthew Woehlke wrote:
>
>> I'm trying to build svn 1.6.0 (and previously 1.5.6, which had the same
>> problems) on one-and-a-half* machines that lack a c99 compiler. This
>> causes problems because svn uses a c99 convention of initializing
>> structs with "non-static" data.
>>
>> The attached patch is needed for compilation to succeed on these
>> platforms. Generally speaking, these problems are:
>> - struct initialization with "non-const" data
>> - comma after last item in an enum
>> - initialize function pointers to NULL (use '0' instead)
>>
>
> Now, you do realize that we only aspire to C89/ANSI-C, right? This is
> documented in our www/hacking.html file. So, some of what you recommend is
> fine because it satisfies both C89 and C99. But some of it, methinks, won't
> pan out well for C89 compilers, namely stuff like this:
>
>
>> diff -ru subversion-1.6.0/subversion/tests/libsvn_client/client-test.c subversion-1.6.0-patched/subversion/tests/libsvn_client/client-test.c
>> --- subversion-1.6.0/subversion/tests/libsvn_client/client-test.c 2008-07-30 08:05:17.000000000 -0700
>> +++ subversion-1.6.0-patched/subversion/tests/libsvn_client/client-test.c 2009-03-25 14:57:04.536775000 -0700
>> @@ -146,7 +146,10 @@
>> apr_array_header_t *targets;
>> apr_getopt_t *os;
>> const int argc = 2;
>> - const char *argv[] = { "opt-test", input, NULL };
>> + const char *argv[3];
>> + argv[0] = "opt-test";
>> + argv[1] = input;
>> + argv[2] = NULL;
>> apr_status_t apr_err;
>> svn_error_t *err;
>>
>
> Cases like this will (I'm guessing) cause problems because they declare
> stack variables after other assignments have been made. We need to move
> those "argv[N] =" lines to after all the variable declarations. Wanna take
> another cut at the patch?
>
Also ... you actually *can* init almost all of argv here. :)
const char *argv[] = { "opt-test", NULL, NULL };
and later:
argv[1] = input;
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1432163
Re: [patch] c99-isms
Posted by "C. Michael Pilato" <cm...@collab.net>.
Matthew Woehlke wrote:
> C. Michael Pilato wrote:
>> You missed a case of declaration-after-assignment, but I'll fix it. Also, I
>> changed all the declarations you touched from merely:
>>
>> struct some_struct;
>>
>> to:
>>
>> struct some_struct = { 0 };
>>
>> which is consistent with the way other code in our codebase inits
>> stack-allocated structures.
>
> You should have said something earlier, I could have done that :-).
True. 'cept I didn't think of it earlier. :-)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1435068
Re: [patch] c99-isms
Posted by Matthew Woehlke <mw...@users.sourceforge.net>.
C. Michael Pilato wrote:
> You missed a case of declaration-after-assignment, but I'll fix it. Also, I
> changed all the declarations you touched from merely:
>
> struct some_struct;
>
> to:
>
> struct some_struct = { 0 };
>
> which is consistent with the way other code in our codebase inits
> stack-allocated structures.
You should have said something earlier, I could have done that :-).
> Thanks for the patch.
Sounds good, thanks again!
--
Matthew
Please do not quote my e-mail address unobfuscated in message bodies.
--
"We're all mad here. I'm mad. You're mad... You must be, or you wouldn't
have come here." -- The Cheshire Cat (from Lewis Carroll's Alice in
Wonderland)
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1434796
Re: [patch] c99-isms
Posted by "C. Michael Pilato" <cm...@collab.net>.
Matthew Woehlke wrote:
> C. Michael Pilato wrote:
>> Matthew Woehlke wrote:
>>> Oops! Actually... I'm not sure how that compiled (given that the
>>> problems I'm having are from non-c99 compilers!) :-). You're right of
>>> course, the assignments should be moved below. Can you do that bit of
>>> cut-and-paste, or do you want a fresh patch?
>> If you give me a fresh patch, you'll be demonstrating a measurable amount of
>> respect for my time. If you give me a fresh patch that applies cleanly
>> against the trunk (as we request all patches to, and as your last one did
>> not), you might even get to be my Favorite Person of the Day. :-)
>
> Well, there is an added whitespace in util.c where I made the change
> causing a conflict; it probably won't backport cleanly either :-).
> Anyway, updated patch (against trunk r36796) attached.
You missed a case of declaration-after-assignment, but I'll fix it. Also, I
changed all the declarations you touched from merely:
struct some_struct;
to:
struct some_struct = { 0 };
which is consistent with the way other code in our codebase inits
stack-allocated structures.
Thanks for the patch.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1434574
Re: [patch] c99-isms
Posted by "C. Michael Pilato" <cm...@collab.net>.
Matthew Woehlke wrote:
> C. Michael Pilato wrote:
>> Matthew Woehlke wrote:
>>> Oops! Actually... I'm not sure how that compiled (given that the
>>> problems I'm having are from non-c99 compilers!) :-). You're right of
>>> course, the assignments should be moved below. Can you do that bit of
>>> cut-and-paste, or do you want a fresh patch?
>> If you give me a fresh patch, you'll be demonstrating a measurable amount of
>> respect for my time. If you give me a fresh patch that applies cleanly
>> against the trunk (as we request all patches to, and as your last one did
>> not), you might even get to be my Favorite Person of the Day. :-)
>
> Well, there is an added whitespace in util.c where I made the change
> causing a conflict; it probably won't backport cleanly either :-).
> Anyway, updated patch (against trunk r36796) attached.
Committed (with tweaks mentioned elsethread) in r36799. Thanks, again.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1435075
Re: [patch] c99-isms
Posted by Matthew Woehlke <mw...@users.sourceforge.net>.
C. Michael Pilato wrote:
> Matthew Woehlke wrote:
>> Oops! Actually... I'm not sure how that compiled (given that the
>> problems I'm having are from non-c99 compilers!) :-). You're right of
>> course, the assignments should be moved below. Can you do that bit of
>> cut-and-paste, or do you want a fresh patch?
>
> If you give me a fresh patch, you'll be demonstrating a measurable amount of
> respect for my time. If you give me a fresh patch that applies cleanly
> against the trunk (as we request all patches to, and as your last one did
> not), you might even get to be my Favorite Person of the Day. :-)
Well, there is an added whitespace in util.c where I made the change
causing a conflict; it probably won't backport cleanly either :-).
Anyway, updated patch (against trunk r36796) attached.
--
Matthew
Please do not quote my e-mail address unobfuscated in message bodies.
--
"We're all mad here. I'm mad. You're mad... You must be, or you wouldn't
have come here." -- The Cheshire Cat (from Lewis Carroll's Alice in
Wonderland)
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1434221
Re: [patch] c99-isms
Posted by "C. Michael Pilato" <cm...@collab.net>.
Matthew Woehlke wrote:
> Oops! Actually... I'm not sure how that compiled (given that the
> problems I'm having are from non-c99 compilers!) :-). You're right of
> course, the assignments should be moved below. Can you do that bit of
> cut-and-paste, or do you want a fresh patch?
If you give me a fresh patch, you'll be demonstrating a measurable amount of
respect for my time. If you give me a fresh patch that applies cleanly
against the trunk (as we request all patches to, and as your last one did
not), you might even get to be my Favorite Person of the Day. :-)
(But if circumstances prevent either, I'm sure I can find time at some point
to make the fixes myself.)
> Thanks for looking at this (and for having pity on me and my pre-c99
> compilers ;-) ).
No problem.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1433188
Re: [patch] c99-isms
Posted by Matthew Woehlke <mw...@users.sourceforge.net>.
C. Michael Pilato wrote:
> Matthew Woehlke wrote:
>> I'm trying to build svn 1.6.0 (and previously 1.5.6, which had the same
>> problems) on one-and-a-half* machines that lack a c99 compiler. This
>> causes problems because svn uses a c99 convention of initializing
>> structs with "non-static" data.
>>
>> The attached patch is needed for compilation to succeed on these
>> platforms. Generally speaking, these problems are:
>> - struct initialization with "non-const" data
>> - comma after last item in an enum
>> - initialize function pointers to NULL (use '0' instead)
>
> Now, you do realize that we only aspire to C89/ANSI-C, right? This is
> documented in our www/hacking.html file. So, some of what you recommend is
> fine because it satisfies both C89 and C99. But some of it, methinks, won't
> pan out well for C89 compilers, namely stuff like this:
>
>> diff -ru subversion-1.6.0/subversion/tests/libsvn_client/client-test.c subversion-1.6.0-patched/subversion/tests/libsvn_client/client-test.c
>> --- subversion-1.6.0/subversion/tests/libsvn_client/client-test.c 2008-07-30 08:05:17.000000000 -0700
>> +++ subversion-1.6.0-patched/subversion/tests/libsvn_client/client-test.c 2009-03-25 14:57:04.536775000 -0700
>> @@ -146,7 +146,10 @@
>> apr_array_header_t *targets;
>> apr_getopt_t *os;
>> const int argc = 2;
>> - const char *argv[] = { "opt-test", input, NULL };
>> + const char *argv[3];
>> + argv[0] = "opt-test";
>> + argv[1] = input;
>> + argv[2] = NULL;
>> apr_status_t apr_err;
>> svn_error_t *err;
>
> Cases like this will (I'm guessing) cause problems because they declare
> stack variables after other assignments have been made. We need to move
> those "argv[N] =" lines to after all the variable declarations. Wanna take
> another cut at the patch?
Oops! Actually... I'm not sure how that compiled (given that the
problems I'm having are from non-c99 compilers!) :-). You're right of
course, the assignments should be moved below. Can you do that bit of
cut-and-paste, or do you want a fresh patch?
Thanks for looking at this (and for having pity on me and my pre-c99
compilers ;-) ).
--
Matthew
Please do not quote my e-mail address unobfuscated in message bodies.
--
"We're all mad here. I'm mad. You're mad... You must be, or you wouldn't
have come here." -- The Cheshire Cat (from Lewis Carroll's Alice in
Wonderland)
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1432480
Re: [patch] c99-isms
Posted by "C. Michael Pilato" <cm...@collab.net>.
Matthew Woehlke wrote:
> I'm trying to build svn 1.6.0 (and previously 1.5.6, which had the same
> problems) on one-and-a-half* machines that lack a c99 compiler. This
> causes problems because svn uses a c99 convention of initializing
> structs with "non-static" data.
>
> The attached patch is needed for compilation to succeed on these
> platforms. Generally speaking, these problems are:
> - struct initialization with "non-const" data
> - comma after last item in an enum
> - initialize function pointers to NULL (use '0' instead)
Now, you do realize that we only aspire to C89/ANSI-C, right? This is
documented in our www/hacking.html file. So, some of what you recommend is
fine because it satisfies both C89 and C99. But some of it, methinks, won't
pan out well for C89 compilers, namely stuff like this:
> diff -ru subversion-1.6.0/subversion/tests/libsvn_client/client-test.c subversion-1.6.0-patched/subversion/tests/libsvn_client/client-test.c
> --- subversion-1.6.0/subversion/tests/libsvn_client/client-test.c 2008-07-30 08:05:17.000000000 -0700
> +++ subversion-1.6.0-patched/subversion/tests/libsvn_client/client-test.c 2009-03-25 14:57:04.536775000 -0700
> @@ -146,7 +146,10 @@
> apr_array_header_t *targets;
> apr_getopt_t *os;
> const int argc = 2;
> - const char *argv[] = { "opt-test", input, NULL };
> + const char *argv[3];
> + argv[0] = "opt-test";
> + argv[1] = input;
> + argv[2] = NULL;
> apr_status_t apr_err;
> svn_error_t *err;
Cases like this will (I'm guessing) cause problems because they declare
stack variables after other assignments have been made. We need to move
those "argv[N] =" lines to after all the variable declarations. Wanna take
another cut at the patch?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1431938