Re: svn commit: r921181 - in /subversion/trunk/subversion/bindings/javahl/native: CopySources.h JNIThreadData.h RevisionRange.h RevpropTable.h

On Mar 9, 2010, at 4:46 PM, Blair Zajac wrote:

> On 03/09/2010 02:41 PM, hwright_at_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
Received on 2010-03-10 00:06:27 CET