Rename remaining float16_t for future proof #25387
Conversation
| #if !defined(OPENCV_HIDE_FLOAT16_T) | ||
| typedef hfloat float16_t; | ||
| #endif |
There was a problem hiding this comment.
you can add c++ version check with __cplusplus macro. See https://stackoverflow.com/questions/2324658/how-to-determine-the-version-of-the-c-standard-used-by-the-compiler
There was a problem hiding this comment.
In original comment there is complete guard condition to avoid using of float16_t in OpenCV code.
__cplusplus
That could be added to C too. No need to make conditions near C++.
There was a problem hiding this comment.
Maksim's solution #25210 (comment):
#if !(defined __STDCPP_FLOAT16_T__) && !(defined __ARM_NEON)
typedef our_pod_fp16_struct float16_t;
#endif
also looks good. Macro __STDCPP_FLOAT16_T__ stands for the existing of std::float16_t in C++23.
There was a problem hiding this comment.
I still want to see __OPENCV_BUILD condition here to avoid usage in OpenCV completely (to avoid mistakes like recent issue).
There was a problem hiding this comment.
I still want to see
__OPENCV_BUILDcondition here to avoid usage in OpenCV completely (to avoid mistakes like recent issue).
Added.
| const float16_t* a = (const float16_t*)_a; | ||
| const float16_t* b = (const float16_t*)_b; | ||
| float16_t* c = (float16_t*)_c; | ||
| #endif |
There was a problem hiding this comment.
Why do we still need this float16_t?
There was a problem hiding this comment.
Because vdupq_n_f16 at line 505 is an macro defined in Clang arm_neon.h (details in #25210 (comment)). In this macro, float16_t is used, so this typedef has to be saved to make vdupq_n_f16 works unless we undo typedef hfloat float16_t in this file.
Note that vdupq_n_f16 in GCC arm_neon.h is an function.
There was a problem hiding this comment.
Maksim's solution may also work, which is to drop typedef hfloat float16_t for ARM platform: #25210 (comment).
There was a problem hiding this comment.
They all are dropped now.
| #if !defined(OPENCV_HIDE_FLOAT16_T) | ||
| typedef hfloat float16_t; | ||
| #endif |
There was a problem hiding this comment.
In original comment there is complete guard condition to avoid using of float16_t in OpenCV code.
__cplusplus
That could be added to C too. No need to make conditions near C++.
fengyuentau
force-pushed
the
complete-float16_t-renaming
branch
from
eb4a8f1
to
567bcff
Compare
* rename remaining internal float16_t to hfloat;
fengyuentau
force-pushed
the
complete-float16_t-renaming
branch
from
4268ca2
to
e20c569
Compare
…enaming Rename remaining float16_t for future proof opencv#25387 Resolves comment: opencv#25217 (comment). `std::float16_t` and `std::bfloat16_t` are introduced since c++23: https://en.cppreference.com/w/cpp/types/floating-point. ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] I agree to contribute to the project under Apache 2 License. - [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [x] The PR is proposed to the proper branch - [x] There is a reference to the original bug report and related work - [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [x] The feature is well documented and sample code can be built with the project CMake
Resolves comment: #25217 (comment).
std::float16_tandstd::bfloat16_tare introduced since c++23: https://en.cppreference.com/w/cpp/types/floating-point.Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.