RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

[Stuart Marks]

> An unconventional patch over an unusual hashcode/equals implementation is a bit 
> too controversial. I'd like to propose a new patch that focus on the problem at 
> hand, that is re-setting the opcode causes the item in HashSet to get lost 
> because of the changed hash bucket. This patch therefore simply remove it from 
> the HashSet before the change and then add it back as if it's a new item after 
> setting the opcode. This patch resolves the issue while leaves BCEL 6.0's 
> hashCode/equals alone.
> 
> Here's the new patch:
> http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev02/

Hi Joe,

This makes a good deal more sense than the previous approach! As you note this 
preserves the consistency of equals() and hashCode(), which is an important 
invariant.

It seems a bit roundabout to have to remove something from the HashSet, mutate 
it, and re-add it, but I don't see an alternative. Mutating it while in the 
HashSet is the original cause of problem. Trying to contort things so that 
objects can remain in the HashSet while being mutated is difficult in general, 
and it seems likely to lead to additional problems. Given this, removing and 
re-adding seems the most sensible approach.

It's a bit fragile in that BranchInstruction now has to "know" that the 
InstructionTarget has a HashSet that requires this to be done. However, I don't 
see a way to avoid this without redesigning the relationships of the objects in 
this area -- which seems out of scope for this change.

Just one comment on the test: there are large test data files 
BCELHashCodeTest1.xsl and BCELHashCodeTest2.xsl. What are these? I'm not asking 
for a tutorial, but rather about what function they serve relative to this bug. 
Just a sentence or two about how BCELHashCodeTest2.xsl causes (I think) the 
generated bytecode to exceed the 64Kbyte method size limit. Splitting this into 
multiple methods requires mutating some of the instructions, and then hilarity 
ensued. :-)

I'm also not sure that BCELHashCodeTest1.xsl is necessary if it passes both with 
and without the patch. But if you think it's valuable, then it's ok for it to 
stay in.

Aside from this, I think this change is good to go.

> (had a nice hike today, and asked the beautiful Lake 22 ;-))

I had to look it up. Lake 22 looks very nice! Please give my regards the next 
time you visit.

s'marks