search

unitsofmeasurement / indriya

JSR 385 - Reference Implementation
Other
119 stars 42 forks source link

Potential stackoverflow in UnitDimension#multiply(Dimension) and #divide(Dimension) #339

Closed andi-huber closed 3 years ago

andi-huber commented 3 years ago

As reported with https://github.com/unitsofmeasurement/uom-se/issues/216 also affects Indriya.

andi-huber commented 3 years ago

fixed with #340

keilw commented 3 years ago

@andi-huber Are we sure, the code in divide() is correct, because the JavaDoc says: @return <code>this.multiply(that.pow(-1))</code>, but the code to divide "foreign" dimensions is now that.divide(this).pow(-1), also matching the JavaDoc comment, but the return tag now has a mismatch. Which is correct?

andi-huber commented 3 years ago

Hi @keilw - the java-doc return tag in UnitDimension#divide(Dimension) does not need to be that specific, as this is an implementation detail of Indriya. We should change that to eg.

@return this / that

which would be in line with what UnitDimension#multiply(Dimension) states, namely @return this * that.

However, the java-doc description of what happens when a 'foreign' Dimension implementation is passed in as an argument, should be specific. So that's what I added yesterday:

If the specified dimension is not a physics dimension, then
<code>that.divide(this).pow(-1)</code> is returned.
andi-huber commented 3 years ago

I took the freedom to push a java-doc correction to 'master'.

keilw commented 3 years ago

As an experienced contributor that's OK thank you.

keilw commented 3 years ago

However, looking at it again, shouldn't @return <code>that / this</code> be like @return <code>this * that</code>? I'll fix that because you may have missed the Java 9 Multi-Release-version in this case, too. Correct me if I'm wrong, but I think it should always be this fist.

andi-huber commented 3 years ago

Correct, I got that java-doc wrong. Thanks for spotting and correction!