Incorporating Gilleain's patch from bug #1201, rebasing on the latest cdk-1.4.x and with more stringent conditions on the number of implicit hydrogens.
With 0002 just wanted to check a couple of things
* the implicit Hydrogen count is null by default so perhaps this should have a null check before the test of 0. Other atom types seem to do atom.getImplicitHydrogenCount() == null || atom.getImplicitHydrogenCount() == 0.
* if the atom typing depends on implicit hydrogens but the implicit hydrogens depends on atom typing - which comes first? I guess it's a similar problem to aromatic information in the atom typing so is this Se.2 atom type only if the implicit hydrogen count is set when it's read?
~~~~~:::Java
if (atom.getImplicitHydrogenCount() == 0) {
IAtomType type = getAtomType("Se.2");
if (isAcceptable(atom, atomContainer, type)) return type;
} else {
IAtomType type = getAtomType("Se.3");
if (isAcceptable(atom, atomContainer, type)) return type;
}
~~~~~
Cheers,
J
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The update will still cause a NPE - using a non null one on the left only works if the you use 'Object.equals()'. I was pretty sure but tested just incase and it does throws a NPE.
Darn... should have tested. I had this exact thing in the past, and am very positive it worked then... I will update it patch again, as you originally suggested...
All good - there on cdk-1.4.x-accepted. It was missing as there were no non-accepted commits on there so a deleted an merged for cdk-1.4.x again to keep the history clean - I forgot to repush when I signed off orginally.
J
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
And my integration patch for Gilleain's start.
I'm happy with 0001 and signed commit is on my accepted branch: https://github.com/johnmay/cdk/tree/cdk-1.4.x-accepted
With 0002 just wanted to check a couple of things
* the implicit Hydrogen count is null by default so perhaps this should have a null check before the test of 0. Other atom types seem to do
atom.getImplicitHydrogenCount() == null || atom.getImplicitHydrogenCount() == 0.* if the atom typing depends on implicit hydrogens but the implicit hydrogens depends on atom typing - which comes first? I guess it's a similar problem to aromatic information in the atom typing so is this Se.2 atom type only if the implicit hydrogen count is set when it's read?
~~~~~:::Java
if (atom.getImplicitHydrogenCount() == 0) {
IAtomType type = getAtomType("Se.2");
if (isAcceptable(atom, atomContainer, type)) return type;
} else {
IAtomType type = getAtomType("Se.3");
if (isAcceptable(atom, atomContainer, type)) return type;
}
~~~~~
Cheers,
J
-> Ah, yes, that should have been the other way around, which does the same as the above but in one go, without the NPE chance:
0 == atom.getImplicitHydrogenCount()
So, I will apply 1.
BTW you removed your cdk-1.4.x-accepted branch.
New 0002 patch.
The update will still cause a NPE - using a non null one on the left only works if the you use 'Object.equals()'. I was pretty sure but tested just incase and it does throws a NPE.
~~~~~:::Java
Integer implicitH = null;
System.out.println(0 == implicitH); // NPE
you can do this though: ~~~~~:::Java Integer.valueOf(0).equals(implicitH)and due to integer caching they'll be no object creation
~~~~~:::Java
public static Integer valueOf(int i) {
if(i >= -128 && i <= IntegerCache.high)
return IntegerCache.cache[i + 128];
else
return new Integer(i);
}
~~~~~
Last edit: John May 2012-11-06
Darn... should have tested. I had this exact thing in the past, and am very positive it worked then... I will update it patch again, as you originally suggested...
Last edit: Egon Willighagen 2012-11-06
I have a feeling it works in groovy...
Both patches are now linked to the "Darn" comment... please review.
Okay thanks - will check them out first thing tomorrow.
All good - there on cdk-1.4.x-accepted. It was missing as there were no non-accepted commits on there so a deleted an merged for cdk-1.4.x again to keep the history clean - I forgot to repush when I signed off orginally.
J
John reviewed it, I tweaked, and it got approved, applied, and pushed.