Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

allowing custom float overflow handling #539

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

allowing custom float overflow handling #539

wants to merge 2 commits into from

Conversation

lucianoviana
Copy link
Contributor

No description provided.

wojciech-adaptive
wojciech-adaptive reviewed Jan 20, 2025
View reviewed changes
artio-codecs/src/test/java/uk/co/real_logic/artio/fields/DecimalFloatDecodingTest.java Outdated
@@ -29,11 +29,16 @@
@RunWith(Parameterized.class)
public class DecimalFloatDecodingTest
{

public static final long MAX_ALLOWED_LONG = 999_999_999_999_999_999L;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make VALUE_MAX_VAL visible instead? At least make this private.

.../test/java/uk/co/real_logic/artio/dictionary/generation/FloatOverflowHandlerDecoderTest.java Outdated
assertFalse(decoder.validate());
}

private void assertPrice(final Object memberIDsGroupDecoder, final Object b) throws Exception
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first parameter name doesn't make sense, the second one could be improved too.

.../test/java/uk/co/real_logic/artio/dictionary/generation/FloatOverflowHandlerDecoderTest.java Outdated
@@ -0,0 +1,98 @@
/*
* Copyright 2021 Adaptive Financial Consulting Ltd.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong year.

.../test/java/uk/co/real_logic/artio/dictionary/generation/FloatOverflowHandlerDecoderTest.java Outdated
import static uk.co.real_logic.artio.dictionary.generation.CodecGenerationWrapper.dictionaryStream;
import static uk.co.real_logic.artio.util.Reflection.get;

import org.junit.Test;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use JUnit 5 for new tests.

artio-codecs/src/main/java/uk/co/real_logic/artio/util/AsciiBuffer.java
DecimalFloat getFloat(DecimalFloat number,
int offset,
int length,
int tagId,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit weird that AsciiBuffer takes a tag number.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I dont do this then there's no way for the tagId to reach the overflow handler.

artio-codecs/src/main/java/uk/co/real_logic/artio/dictionary/generation/CodecConfiguration.java Outdated
{
//noinspection unchecked
decimalFloatOverflowHandler = (Class<? extends DecimalFloatOverflowHandler>)Class.forName(
floatOverflowHandler);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to actually load that class instead of using a String fqcn?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doing this during codec generation time will fail-fast in case the class name is invalid or it's not reachable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which means you need to have in classpath while generating code, compiling the generated code will validate that anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see - will change this then

artio-codecs/src/main/java/uk/co/real_logic/artio/dictionary/generation/DecoderGenerator.java Outdated
null);
}

DecoderGenerator(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update callers instead of adding a new constructor? It's internal, so that should be simpler.

artio-codecs/src/main/java/uk/co/real_logic/artio/util/float_parsing/DecimalFloatParser.java Outdated
final int dotIndex,
final int tagId)
{
return overflowHandler.handleOverflow(charReader.asString(data, offset, length),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will allocate a substring, plus the API forces implementations to allocate a DecimalFloat, why not pass the existing objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point - cheers

artio-codecs/src/main/java/uk/co/real_logic/artio/util/float_parsing/DecimalFloatParser.java Outdated
return updateValue(number, workingScale, exponent, timesNeg, negative, value);
}

private static DecimalFloat updateValue(final DecimalFloat number, final int workingScale, final int exponent,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please format consistently, ideally every parameter on its own line if breaking is needed.

@lucianoviana lucianoviana force-pushed the luc/hy-52 branch 2 times, most recently from 2aa9924 to 544fe86 Compare January 21, 2025 15:30
wojciech-adaptive
wojciech-adaptive reviewed Jan 23, 2025
View reviewed changes
...ecs/src/main/java/uk/co/real_logic/artio/util/float_parsing/DecimalFloatOverflowHandler.java Outdated
* @return instance of DecimalFloat to be use
* @param <Data> generic buffer
*/
<Data> DecimalFloat handleOverflow(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That still forces the implementations to allocate the result, why not pass the existing instance in?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope it's in a better shape now

artio-codecs/src/main/java/uk/co/real_logic/artio/dictionary/generation/CodecConfiguration.java Outdated
@@ -94,6 +97,24 @@ public CodecConfiguration outputPath(final String outputPath)
return this;
}

/**
* Sets the full qualified class name of a DecimalFloat overflow handler.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to mention what interface it needs to implement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wojciech-adaptive wojciech-adaptive wojciech-adaptive left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lucianoviana @wojciech-adaptive