Repository navigation
Conversation
|
Can you remove the test please - not needed or useful. |
|
This one is okay: @Test
void unsupportedElement() {
Assertions.assertThrows(IllegalArgumentException.class,
() -> AtomValenceTool.getValence(new Atom("Fe")));
}But the one which just mirrors all the data is slop. |
|
@kmansouri Note it did not build because of the new exception |
There was a problem hiding this comment.
First, I think the move to a static, immutable collection is a good improvement, thanks!
A few requests, see comments on the code below.
The failing test is interesting, as the original code does not seem to define a valence for Pt. The problem is that the patch changes the API, by introducing a new exception. API changes should not happen in a stable series. I now think this is exactly why we now have a failing test. Let me think a bit more about the right "fix" here: 1. restore the previous API, or 2. fix the downstream code and/or test.
Maybe the fix is just to add the defined valence for Pt.
| @@ -0,0 +1,46 @@ | |||
| /* Copyright (C) 2026 Kamel Mansouri | |||
There was a problem hiding this comment.
Please add some further contact information. Traditionally, your email address. This is important, allowing people to contact you later on. Format:
Copyright (C) 2026 Kamel Mansouri <your@email.org>
| 5, 6, 7, 1, 2, 3, 4, 5, 6, 7, 1, 2, 3, 4, 5, 6, 7, 1, 2, 2, 2, 2}; | ||
|
|
||
| for (int i = 0; i < symbols.length; i++) { | ||
| Assertions.assertEquals(valences[i], AtomValenceTool.getValence(new Atom(symbols[i])), symbols[i]); |
There was a problem hiding this comment.
If not mistaken, the third parameter is an error message. But just listing the symbol is not very informative. I would suggest something like
"Unexpected valence for atom with symbol " + symbols[i]
| @Test | ||
| void unsupportedElement() { | ||
| Assertions.assertThrows(IllegalArgumentException.class, | ||
| () -> AtomValenceTool.getValence(new Atom("Fe"))); |
There was a problem hiding this comment.
I guess this is technically right. Should Fe not have a valence defined? Like Pt?
|
Thanks @johnmay and @egonw. I addressed the review comments in the latest commit:
I left Fe/Pt coverage unchanged so this PR remains focused on thread-safe |
Summary
Replace the lazily initialized mutable valence map in
AtomValenceToolwith asafely published immutable map initialized during class loading.
The previous implementation assigned the shared map before populating it. A
concurrent first call could therefore observe a non-null but incomplete table
and fail while unboxing a missing value.
Changes
a nonzero valence.
This issue was found while porting PaDEL-Descriptor from CDK 1.x to CDK 2.13,
but the change and test are independent of PaDEL.
Fixes #1308.
Testing
against the patched class with no failure.
duplicating the implementation's valence values.