You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2004/03/11 23:52:14 UTC
[Fwd: Re: [PATCH] stdlib removed from svn_types.h]
[I hope you don't mind me forwarding this to the "dev@" list. I want to keep the discussion there, but I forgot to set "Reply-To" on the previous message I sent.]
-------- Original Message --------
Subject: Re: [PATCH] stdlib removed from svn_types.h
Date: Thu, 11 Mar 2004 18:49:13 -0300
From: H. Hernan Moraldo <ga...@moraldo.com.ar>
Reply-To: games@moraldo.com.ar
Organization: Moraldo Games
To: Julian Foad <ju...@btopenworld.com>
References: <10...@linmachine> <40...@btopenworld.com>
El mié, 10-03-2004 a las 14:11, Julian Foad escribió:
> H. Hernan Moraldo wrote:
> > Remove the #include <stdlib.h> line from svn_types.h, and add that line
> > to any other files that actually need it.
> [...]
> > On subversion/include/svn_types.h there were some lines telling:
> >
> > /* ### this should go away, but it causes too much breakage right now */
> > #include <stdlib.h>
>
> Very nearly OK ... but there is one macro that needs stdlib.h:
>
> /** Convert null-terminated C string @a str to a revision number. */
> #define SVN_STR_TO_REV(str) ((svn_revnum_t) atol(str))
According to the replies, I think there are three choices:
1- To keep it all just as it is now
2- To covert SVN_STR_TO_REV to a function, and that would be major
breakage for real, just as pointed in the original commentary. If you
say this is the preferred option, I can try working on this patch.
3- The third option is the one drafted on the patch attached: we don't
remove stdlib from svn_types.h, but we stop assuming it is included
automatically by including svn_types.h on the other source files. I
think this makes for better code, but couldn't know if you agree on
this.
Opinions?
Best regards,
--
H. Hernan Moraldo
Moraldo Games
http://games.moraldo.com.ar/
Re: [PATCH] including stdlib in source files
Posted by "H. Hernan Moraldo" <ga...@moraldo.com.ar>.
> This patch is incomplete. Those are only a few of the files that should include <stdlib.h> directly. There are many more. Remove "#include <stdlib.h>" from svn_types.h and then look for warnings like this:
>
> /home/julianfoad/src/subversion/subversion/libsvn_delta/path_driver.c:158: warning: implicit declaration of function `qsort'
Sorry, I hadn't noticed it. I'll correct it and send another patch soon.
Best regards,
--
H. Hernan Moraldo
Moraldo Games
http://games.moraldo.com.ar/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] including stdlib in source files
Posted by Julian Foad <ju...@btopenworld.com>.
H. Hernan Moraldo wrote:
> Log message:
>
> Include stdlib.h on files that were including this indirectly, through
> svn_types.h.
>
> * subversion/svnadmin/main.c
> * subversion/svnlook/main.c
> * subversion/clients/cmdline/main.c
> * subversion/tests/libsvn_subr/target-test.c
> * subversion/svndumpfilter/main.c
> * subversion/svnserve/main.c
> * subversion/svnversion/main.c
> stdlib.h is now directly #included on these
> files.
This patch is incomplete. Those are only a few of the files that should include <stdlib.h> directly. There are many more. Remove "#include <stdlib.h>" from svn_types.h and then look for warnings like this:
/home/julianfoad/src/subversion/subversion/libsvn_delta/path_driver.c:158: warning: implicit declaration of function `qsort'
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] including stdlib in source files
Posted by "H. Hernan Moraldo" <ga...@moraldo.com.ar>.
Log message:
Include stdlib.h on files that were including this indirectly, through
svn_types.h.
* subversion/svnadmin/main.c
* subversion/svnlook/main.c
* subversion/clients/cmdline/main.c
* subversion/tests/libsvn_subr/target-test.c
* subversion/svndumpfilter/main.c
* subversion/svnserve/main.c
* subversion/svnversion/main.c
stdlib.h is now directly #included on these
files.
(end of log)
The patch is the same that in my latest message, but here is again, with
a new mail subject and its respective log message. Now I'll look for
more significative changes to the SVN code.
Best regards,
--
H. Hernan Moraldo
Moraldo Games
http://games.moraldo.com.ar/
Re: [PATCH] stdlib removed from svn_types.h
Posted by "H. Hernan Moraldo" <ga...@moraldo.com.ar>.
> > I stopped working on this because it didn't
> > really seem worthwhile to do half of the job. If I can't find all of
> > the missing and redundant includes, then I am not sure whether it is
> > really worth fixing any of them. Maybe it is.
>
> I went to bed and then, after a few minutes, got up again, realising that I'd been too negative. There's no reason not to commit incremental improvements. I would be happy to commit your patch after I have verified it (but I can't do that until next week).
Thanks for your reply, that patch was only a 'draft' to explain better
the concept. Now I'll check all is ok, and if so, will send a new patch
with its corresponding log text, etc etc.
Best regards,
--
H. Hernan Moraldo
Moraldo Games
http://games.moraldo.com.ar/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] stdlib removed from svn_types.h
Posted by Julian Foad <ju...@btopenworld.com>.
Julian Foad wrote:
>> From: H. Hernan Moraldo <ga...@moraldo.com.ar>
>>
>> 3- The third option is the one drafted on the patch attached: we don't
>> remove stdlib from svn_types.h, but we stop assuming it is included
>> automatically by including svn_types.h on the other source files. I
>> think this makes for better code, but couldn't know if you agree on
>> this.
>
[...]
> I stopped working on this because it didn't
> really seem worthwhile to do half of the job. If I can't find all of
> the missing and redundant includes, then I am not sure whether it is
> really worth fixing any of them. Maybe it is.
I went to bed and then, after a few minutes, got up again, realising that I'd been too negative. There's no reason not to commit incremental improvements. I would be happy to commit your patch after I have verified it (but I can't do that until next week).
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] stdlib removed from svn_types.h
Posted by Julian Foad <ju...@btopenworld.com>.
> From: H. Hernan Moraldo <ga...@moraldo.com.ar>
>
> El mié, 10-03-2004 a las 14:11, Julian Foad escribió:
>
>> H. Hernan Moraldo wrote:
>> > Remove the #include <stdlib.h> line from svn_types.h, and add that line
>> > to any other files that actually need it.
>> [...]
>> > On subversion/include/svn_types.h there were some lines telling:
>> > > /* ### this should go away, but it causes too much breakage right now */
>> > #include <stdlib.h>
>>
>> Very nearly OK ... but there is one macro that needs stdlib.h:
>>
>> /** Convert null-terminated C string @a str to a revision number. */
>> #define SVN_STR_TO_REV(str) ((svn_revnum_t) atol(str))
>
> According to the replies, I think there are three choices:
>
> 1- To keep it all just as it is now
>
> 2- To covert SVN_STR_TO_REV to a function, and that would be major
> breakage for real, just as pointed in the original commentary. If you
> say this is the preferred option, I can try working on this patch.
Ideally this should be done, but I see two major issues. First, in which library would the implementation live, and would that meet our compatibility guarantees? Second, if we replace this function-like macro with a true function of the same name, would we consider that to be source-code-compatible enough, and would the same (all-capitals) name be aesthetically acceptable? (I am not directly asking you these questions, just saying that they would have to be answered if we try to do this.)
> 3- The third option is the one drafted on the patch attached: we don't
> remove stdlib from svn_types.h, but we stop assuming it is included
> automatically by including svn_types.h on the other source files. I
> think this makes for better code, but couldn't know if you agree on
> this.
Yes, I think that is a good thing to do and makes for better code. If a source file is directly using facilities from some header, it should directly include that header. This applies not just for <stdlib.h>, but to all headers. In fact, I have already started doing this for Subversion headers. I wrote the script attached ("includes.sh") to help find missing and redundant "#include" lines. It is by no means comprehensive, but it does find certain cases. The commented out code finds those cases where the header file is a nice tidy one with all declearations having the same prefix that matches the header file's name.
The results that I have obtained so far are in the second attachment ("tidy-includes.patch"). I stopped working on this because it didn't really seem worthwhile to do half of the job. If I can't find all of the missing and redundant includes, then I am not sure whether it is really worth fixing any of them. Maybe it is.
Options 2 and 3 are not mutually exclusive, of course. Both of them are good things to do, in principle. In practice, I don't know.
- Julian