Re: [Swig-devel] How to insert into DohHash without breaking iterating over it?

[William S F. <ws...@fu...>]

On 08/12/11 22:39, Vadim Zeitlin wrote:
> On Sat, 19 Nov 2011 22:43:55 +0000 William S Fulton<ws...@fu...>  wrote:
>
> WSF>  On 19 November 2011 13:36, Vadim Zeitlin<vz...@ze...>  wrote:
> WSF>  >  On Fri, 18 Nov 2011 19:59:52 +0000 William S Fulton<ws...@fu...>  wrote:
> WSF>  >
> WSF>  >  WSF>  Your idea of creating a temporary hash is probably the best approach. A
> WSF>  >  WSF>  temporary List might be faster, but I doubt there will be much in it it
> WSF>  >  WSF>  because the temporary hash/list is probably not going to contain many
> WSF>  >  WSF>  members. The lookup will also need to look in the temporary hash.
> WSF>  >
> WSF>  >   Sorry, why? As far as I understand the code, we iterate to find all the
> WSF>  >  elements with the prefix corresponding to the base class and insert only
> WSF>  >  elements with the prefix corresponding to the derived class. So the
> WSF>  >  temporary hash/list would never have any entries we're interested in
> WSF>  >  because we only want those starting with "base::" while all temporary hash
> WSF>  >  elements start with "derived::". Am I missing something?
> WSF>  >
> WSF>  I'll get back to you on this, I may have got something wrong as I
> WSF>  didn't spend that long looking at it.
>
>   Hello,
>
>   I'm not sure if you were still going to look at this so I just went ahead
> and committed a fix for this as r12865 in the meanwhile.
>
> WSF>  DOH objects are reference counted, so I'm sure you'll find Delete()
> WSF>  removes a reference count and Setattr() adds a reference count.
>
>   Doh! (sic)
>
>   I have totally missed this fact. But this means that my alternative
> solution using a temporary hash map is much simpler than I thought as I
> don't need to do any memory management at all and the objects are
> effectively being moved from one hash to another. The only overhead is that
> the keys are not shared AFAICS but I can't do anything about this.
>
>   So finally I checked in the version using the temporary hash rather than
> the one using a temporary list of keys for iteration as it is slightly
> faster in my testing. I also ran it under valgrind which didn't find any
> errors (although it took half of eternity to run and did find 1.5MB of
> memory leaks -- but this was the case without my changes too, the number of
> leaked blocks didn't change).
>
>   Please let me know if you see anything wrong with these changes, thanks,

Hi Vadim

Unfortunately I can see that something is broken, but am not quite sure 
how you've managed it. What I normally do with changes in the core like 
this is to run the partialcheck make targets, eg 'make 
partialcheck-java-test-suite' and copy the results somewhere and run 
again with the change, then do a directory diff using meld, or whatever 
GUI diff tool takes your fancy. With your changes, I see a couple of 
changes, eg in Python, the contract and director_frob tests generate 
different code. In Java, I also see minherit2 has changed.

I looked at the output with the "-debug-module 3" debugging option 
turned on and you can see that the features are different. In this case 
for RemoteMpe:

       | feature:java:methodmodifiers - "abstract public"

instead of:

       | feature:java:methodmodifiers - "public"

In this case, the original code gave correct results and new code is 
wrong as in this testcase, the derived class (RemoteMpe) has a feature 
which overrides the one in the base class.

Could you investigate please?

Thanks
William