correct licensing and incorporation of FastMath#49122
Conversation
this resolves incorrectly licensed code in elastic#49009. ESSloppyMath is removed in favor of preserving as much of the original FastMath as possible. Since no additional methods are introduced in ESSloppyMath, this abstraction is removed.
|
Pinging @elastic/es-analytics-geo (:Analytics/Geo) |
jasontedor
left a comment
There was a problem hiding this comment.
This is good. Thanks. I think I like a level of indirection on top of FastMath (and make it package private) though so that we aren't tempted to add code to it that doesn't fall under the license at the top of the file. Maybe I'm overthinking it, and I can be convinced otherwise.
|
I totally get the reasoning, but it feels like overkill given where that there is no other code that would be defined in ESSloppyMath, other than these thin wrappers. Would an alternative to introducing a wrapper be moving FastMath to its original package |
|
I'm fine without the level of indirection if we add a top-level comment to the class that additions/modifications to it should only be those that come from the original source. |
|
after working on some other code, I see value in introducing more helper methods like an |
|
That’s great, thanks! |
jasontedor
left a comment
There was a problem hiding this comment.
I left one comment that requires attention before merging, otherwise LGTM.
libs/core/src/main/java/org/elasticsearch/common/util/FastMath.java
Outdated
Show resolved
Hide resolved
|
@elasticmachine update branch |
|
The backport PR seems to have been merged. I'm therefore removing the backport pending label here. Please shout if this is incorrect |
this resolves incorrectly licensed code in #49009.
ESSloppyMath is removed in favor of preserving as much of the original
FastMath as possible. Since no additional methods are introduced in
ESSloppyMath, this abstraction is removed.
thanks @jasontedor for assisting to resolve this.