You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by hw...@apache.org on 2010/03/09 23:41:16 UTC
svn commit: r921181 - in
/subversion/trunk/subversion/bindings/javahl/native: CopySources.h
JNIThreadData.h RevisionRange.h RevpropTable.h
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.
* subversion/bindings/javahl/native/JNIThreadData.h,
subversion/bindings/javahl/native/RevisionRange.h,
subversion/bindings/javahl/native/RevpropTable.h,
subversion/bindings/javahl/native/CopySources.h:
Remove Pool class declaration and just include the Pool header.
Modified:
subversion/trunk/subversion/bindings/javahl/native/CopySources.h
subversion/trunk/subversion/bindings/javahl/native/JNIThreadData.h
subversion/trunk/subversion/bindings/javahl/native/RevisionRange.h
subversion/trunk/subversion/bindings/javahl/native/RevpropTable.h
Modified: subversion/trunk/subversion/bindings/javahl/native/CopySources.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/CopySources.h?rev=921181&r1=921180&r2=921181&view=diff
==============================================================================
--- subversion/trunk/subversion/bindings/javahl/native/CopySources.h (original)
+++ subversion/trunk/subversion/bindings/javahl/native/CopySources.h Tue Mar 9 22:41:16 2010
@@ -30,7 +30,7 @@
#include <jni.h>
#include <apr_tables.h>
-class SVN::Pool;
+#include "Pool.h"
/**
* A container for our copy sources, which can convert them into an
Modified: subversion/trunk/subversion/bindings/javahl/native/JNIThreadData.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/JNIThreadData.h?rev=921181&r1=921180&r2=921181&view=diff
==============================================================================
--- subversion/trunk/subversion/bindings/javahl/native/JNIThreadData.h (original)
+++ subversion/trunk/subversion/bindings/javahl/native/JNIThreadData.h Tue Mar 9 22:41:16 2010
@@ -29,8 +29,8 @@
#include <jni.h>
#include "JNIUtil.h"
+
struct apr_threadkey_t;
-class SVN::Pool;
/**
* This class implements thread local storage for JNIUtil.
Modified: subversion/trunk/subversion/bindings/javahl/native/RevisionRange.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/RevisionRange.h?rev=921181&r1=921180&r2=921181&view=diff
==============================================================================
--- subversion/trunk/subversion/bindings/javahl/native/RevisionRange.h (original)
+++ subversion/trunk/subversion/bindings/javahl/native/RevisionRange.h Tue Mar 9 22:41:16 2010
@@ -30,7 +30,7 @@
#include <jni.h>
#include "svn_types.h"
-class SVN::Pool;
+#include "Pool.h"
/**
* A container for our copy sources, which can convert them into an
Modified: subversion/trunk/subversion/bindings/javahl/native/RevpropTable.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/RevpropTable.h?rev=921181&r1=921180&r2=921181&view=diff
==============================================================================
--- subversion/trunk/subversion/bindings/javahl/native/RevpropTable.h (original)
+++ subversion/trunk/subversion/bindings/javahl/native/RevpropTable.h Tue Mar 9 22:41:16 2010
@@ -31,8 +31,6 @@
#include "Pool.h"
struct apr_hash_t;
-struct svn_error_t;
-class SVN::Pool;
#include "Path.h"
#include <map>
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
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 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