-
Notifications
You must be signed in to change notification settings - Fork 575
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.
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
Allow to customize float literals in output #2230
Allow to customize float literals in output #2230
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned, this is simple enough, see comments though.
{ | ||
public: | ||
virtual ~FloatFormatter() = default; | ||
virtual std::string format_float(float value) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is one needed for fp16, or will that hit the format_float part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently float16_t
will be converted to float
before be converted to string:
Lines 5910 to 5913 in 42aac91
string CompilerGLSL::convert_half_to_string(const SPIRConstant &c, uint32_t col, uint32_t row) | |
{ | |
string res; | |
float float_value = c.scalar_f16(col, row); |
I think format_float16
is not needed.
27c99f3
to
aaddc49
Compare
I use "build" subdirectory to generated CMake files, so I added it to .gitignore. Please point out if it conflicts with something. |
aaddc49
to
20dd53b
Compare
This PR does not change the current behavior. It only works when invoking
set_float_formatter()
with initiative.I use
std::shared_ptr
to store aFloatFormatter
virtual base object, because it can use customized deleter (compare tostd::unique_ptr
). Would it be better to just use a raw pointerFloatFormatter*
and require the user to keep it valid?Related issues: #2132 #1599