You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Blair Zajac <bl...@orcaware.com> on 2010/03/09 22:46:17 UTC
Re: svn commit: r921181 - in /subversion/trunk/subversion/bindings/javahl/native:
CopySources.h JNIThreadData.h RevisionRange.h RevpropTable.h
On 03/09/2010 02:41 PM, hwright@apache.org wrote:
> Author: hwright
> Date: Tue Mar 9 22:41:16 2010
> New Revision: 921181
>
> URL: http://svn.apache.org/viewvc?rev=921181&view=rev
> Log:
> JavaHL: Fix a few header files to avoid a redundant declaration of SVN::Pool.
> Instead of declaring the class (when it might also be declared previously by
> some other header file), just include the header if needed. The header already
> has the required #ifdef protection, and it doesn't cost much to parse it
> again anyway.
Standard practice is to only include header files one needs for the
definition. Plus, it's not just that, but the cost of recompiling
everything that then #include's that header file if you touch Pooo.h
I suggest just reverse merging this change, as while it's not the cost
that matters (which is low as you state), but just being correct about it.
Regards,
Blair
Re: svn commit: r921181 - in /subversion/trunk/subversion/bindings/javahl/native:
CopySources.h JNIThreadData.h RevisionRange.h RevpropTable.h
Posted by Blair Zajac <bl...@orcaware.com>.
On 03/09/2010 03:05 PM, Hyrum K. Wright wrote:
>
> On Mar 9, 2010, at 4:46 PM, Blair Zajac wrote:
>
>> On 03/09/2010 02:41 PM, hwright@apache.org wrote:
>>> Author: hwright
>>> Date: Tue Mar 9 22:41:16 2010
>>> New Revision: 921181
>>>
>>> URL: http://svn.apache.org/viewvc?rev=921181&view=rev
>>> Log:
>>> JavaHL: Fix a few header files to avoid a redundant declaration of SVN::Pool.
>>> Instead of declaring the class (when it might also be declared previously by
>>> some other header file), just include the header if needed. The header already
>>> has the required #ifdef protection, and it doesn't cost much to parse it
>>> again anyway.
>>
>> Standard practice is to only include header files one needs for the definition. Plus, it's not just that, but the cost of recompiling everything that then #include's that header file if you touch Pooo.h
>
> I try to stay as far away from Pooo.h has possible. :)
Me too. I should read what I write :)
> SVN::Pool;
>
> The last line creates a warning, but only occasionally, depending on where the declaration falls in relation to various other includes (which are often in other files). The available options are:
> 1) Status quo
> 2) r921181
> 3) Wrap all SVN::Pool declarations with #ifdef's to avoid duplicate declaration warnings
> 4) Audit the entire header file corpus to ensure all declarations are properly ordered.
>
> I chose (2), which gives the benefits desired with the least amount of work. The problem really goes much farther than SVN::Pool: we include a number of our header files in other header files. To properly solve the problem requires (4); reverting r921181 doesn't really fix anything, it's just shuffling deck chairs.
>
> If anybody wants to tackle (4), please feel free.
OK. Thanks for letting me know the choices. Definitely, (2) is the
easiest.
Blair
Re: svn commit: r921181 - in /subversion/trunk/subversion/bindings/javahl/native: CopySources.h JNIThreadData.h RevisionRange.h RevpropTable.h
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Mar 9, 2010, at 4:46 PM, Blair Zajac wrote:
> On 03/09/2010 02:41 PM, hwright@apache.org wrote:
>> Author: hwright
>> Date: Tue Mar 9 22:41:16 2010
>> New Revision: 921181
>>
>> URL: http://svn.apache.org/viewvc?rev=921181&view=rev
>> Log:
>> JavaHL: Fix a few header files to avoid a redundant declaration of SVN::Pool.
>> Instead of declaring the class (when it might also be declared previously by
>> some other header file), just include the header if needed. The header already
>> has the required #ifdef protection, and it doesn't cost much to parse it
>> again anyway.
>
> Standard practice is to only include header files one needs for the definition. Plus, it's not just that, but the cost of recompiling everything that then #include's that header file if you touch Pooo.h
I try to stay as far away from Pooo.h has possible. :)
In all seriousness, when working in the Java bindings, I spend a fair amount of time tracking down warnings and making sure they aren't errors. g++ can be verbose as it is; I'm just trying to make my life a little easier by eliminating a number of superfluous warnings. Since I seem to be the only person hacking JavaHL these days, I think this little bit of selfish indulgence is justified.
> I suggest just reverse merging this change, as while it's not the cost that matters (which is low as you state), but just being correct about it.
This point I understand. The problem happens when the class isn't declared in the proper order in the header sequence, and we end up with something that expands to:
SVN::Pool {
...
};
...
SVN::Pool;
The last line creates a warning, but only occasionally, depending on where the declaration falls in relation to various other includes (which are often in other files). The available options are:
1) Status quo
2) r921181
3) Wrap all SVN::Pool declarations with #ifdef's to avoid duplicate declaration warnings
4) Audit the entire header file corpus to ensure all declarations are properly ordered.
I chose (2), which gives the benefits desired with the least amount of work. The problem really goes much farther than SVN::Pool: we include a number of our header files in other header files. To properly solve the problem requires (4); reverting r921181 doesn't really fix anything, it's just shuffling deck chairs.
If anybody wants to tackle (4), please feel free.
-Hyrum
Re: svn commit: r921181 - in /subversion/trunk/subversion/bindings/javahl/native: CopySources.h JNIThreadData.h RevisionRange.h RevpropTable.h
Posted by "Hyrum K. Wright" <hw...@apache.org>.
On Mar 9, 2010, at 4:46 PM, Blair Zajac wrote:
> On 03/09/2010 02:41 PM, hwright@apache.org wrote:
>> Author: hwright
>> Date: Tue Mar 9 22:41:16 2010
>> New Revision: 921181
>>
>> URL: http://svn.apache.org/viewvc?rev=921181&view=rev
>> Log:
>> JavaHL: Fix a few header files to avoid a redundant declaration of SVN::Pool.
>> Instead of declaring the class (when it might also be declared previously by
>> some other header file), just include the header if needed. The header already
>> has the required #ifdef protection, and it doesn't cost much to parse it
>> again anyway.
>
> Standard practice is to only include header files one needs for the definition. Plus, it's not just that, but the cost of recompiling everything that then #include's that header file if you touch Pooo.h
I try to stay as far away from Pooo.h has possible. :)
In all seriousness, when working in the Java bindings, I spend a fair amount of time tracking down warnings and making sure they aren't errors. g++ can be verbose as it is; I'm just trying to make my life a little easier by eliminating a number of superfluous warnings. Since I seem to be the only person hacking JavaHL these days, I think this little bit of selfish indulgence is justified.
> I suggest just reverse merging this change, as while it's not the cost that matters (which is low as you state), but just being correct about it.
This point I understand. The problem happens when the class isn't declared in the proper order in the header sequence, and we end up with something that expands to:
SVN::Pool {
...
};
...
SVN::Pool;
The last line creates a warning, but only occasionally, depending on where the declaration falls in relation to various other includes (which are often in other files). The available options are:
1) Status quo
2) r921181
3) Wrap all SVN::Pool declarations with #ifdef's to avoid duplicate declaration warnings
4) Audit the entire header file corpus to ensure all declarations are properly ordered.
I chose (2), which gives the benefits desired with the least amount of work. The problem really goes much farther than SVN::Pool: we include a number of our header files in other header files. To properly solve the problem requires (4); reverting r921181 doesn't really fix anything, it's just shuffling deck chairs.
If anybody wants to tackle (4), please feel free.
-Hyrum