Skip to content
This repository was archived by the owner on Dec 20, 2023. It is now read-only.

add Float number print in DataElement::Parser::ParseData#576

Open
yunhanw-google wants to merge 1 commit into
masterfrom
feature/add_float_point_print
Open

add Float number print in DataElement::Parser::ParseData#576
yunhanw-google wants to merge 1 commit into
masterfrom
feature/add_float_point_print

Conversation

@yunhanw-google

Copy link
Copy Markdown
Contributor

No description provided.

@yunhanw-google yunhanw-google requested a review from robszewczyk May 4, 2020 21:30

@gerickson gerickson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are we now actively using floating point types in schema? If so, which one(s)?

@yunhanw-google yunhanw-google force-pushed the feature/add_float_point_print branch from 3081c2f to 0669aac Compare May 4, 2020 22:33
@yunhanw-google

Copy link
Copy Markdown
Contributor Author

Are we now actively using floating point types in schema? If so, which one(s)?

https://github.com/openweave/openweave-core/blob/master/src/test-apps/schema/nest/test/trait/TestATrait.h#L180
We have several internal/external traits which is using float

@yunhanw-google

Copy link
Copy Markdown
Contributor Author

Just update the corresponding test in this PR. thanks

err = aReader.Get(value_fp);
SuccessOrExit(err);

PRETTY_PRINT_SAMELINE("%lf, ", value_fp);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This may run into problems on platforms where printf doesn't support float.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

maybe we can add the feature flag on it? WEAVE_CONFIG_SUPPORT_FLOAT_PRINT?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants