make detect encoding functions constexpr - #920
shikharish wants to merge 6 commits into
Conversation
Signed-off-by: Shikhar <shikharish05@gmail.com>
187b3c6 to
83388fb
Compare
Signed-off-by: Shikhar <shikharish05@gmail.com>
|
cc: @lemire @pauldreik |
|
@shikharish thanks. This will nicely close our gap. Let us include this in our next release. |
pauldreik
left a comment
There was a problem hiding this comment.
Thanks for the PR! I am in a hurry so sorry for not doing a thorough review.
I think it is mostly fine but I have some questions.
| simdutf_warn_unused encoding_type check_bom(const uint8_t *byte, size_t length); | ||
| simdutf_warn_unused encoding_type check_bom(const char *byte, size_t length); | ||
| template <typename BytePtr> | ||
| simdutf_warn_unused inline simdutf_constexpr14 encoding_type |
There was a problem hiding this comment.
let's use a macro guarded c++20 concept here to restrict the bytepr.
| return encoding_type::unspecified; | ||
| } | ||
|
|
||
| simdutf_warn_unused inline encoding_type check_bom(const char *byte, |
There was a problem hiding this comment.
should be really_inline
| #include <simdutf/scalar/utf32_to_utf8/utf32_to_utf8.h> | ||
| #include <simdutf/scalar/utf32_to_utf8/valid_utf32_to_utf8.h> | ||
| #include <simdutf/scalar/utf8.h> | ||
| #include <simdutf/scalar/detect.h> |
There was a problem hiding this comment.
why is this included twice?
| #include <simdutf/scalar/utf8.h> | ||
| #include <simdutf/scalar/utf16.h> | ||
| #include <simdutf/scalar/utf32.h> | ||
| #include <simdutf/scalar/detect.h> |
There was a problem hiding this comment.
it is ok to move around the includes, but remove those that are duplicates
| #include <simdutf/scalar/utf32.h> | ||
| #include <simdutf/scalar/detect.h> | ||
|
|
||
| #if SIMDUTF_SPAN |
| * of units processed if successful. | ||
| */ | ||
| simdutf_warn_unused result | ||
| simdutf_warn_unused simdutf_constexpr23 result |
| } | ||
|
|
||
| template <endianness big_endian> | ||
| template <endianness big_endian, typename InputPtr> |
There was a problem hiding this comment.
please add a macro guarded c++20 concept check.
There was a problem hiding this comment.
(and reused the already existing concept and style)
Signed-off-by: Shikhar <shikharish05@gmail.com>
Signed-off-by: Shikhar <shikharish05@gmail.com>
2b17857 to
b831db8
Compare
|
@shikharish Could you have a look at @pauldreik's comments ? There are a few that are apparently outstanding (meaning that it is unclear whether you answered or changed the code accordingly). |
|
@lemire I made all the requested changes. There were a few things I misunderstood before but I've fixed them in the last few commits. |
|
@pauldreik Can you check that your concerns were addressed? |
| #if SIMDUTF_CPLUSPLUS20 | ||
| #include <concepts> | ||
| template <typename BytePtr> | ||
| concept check_bom_byteptr = requires(BytePtr p, size_t i) { |
There was a problem hiding this comment.
please look at the other constexpr changes recently merged and reuse the already existing concepts.
| } | ||
|
|
||
| template <endianness big_endian> | ||
| template <endianness big_endian, typename InputPtr> |
There was a problem hiding this comment.
(and reused the already existing concept and style)
Signed-off-by: Shikhar <shikharish05@gmail.com>
|
@pauldreik Please review. |
|
@shikharish when you get review comments, please go through them one by one and fix/answer them. otherwise I have to go through everything again. there are still issues left from earlier comments. what is also left to do:
|
| #include <iostream> | ||
| #include <array> |
There was a problem hiding this comment.
these don't seem to be used.
| #if SIMDUTF_SPAN | ||
| template <endianness big_endian, simdutf::detail::indexes_into_utf16 InputPtr> | ||
| #else | ||
| template <endianness big_endian, typename InputPtr> | ||
| #endif |
There was a problem hiding this comment.
template <endianness big_endian, typename InputPtr>
#if SIMDUTF_CPLUSPLUS20
requires simdutf::detail::indexes_into_utf16<InputPtr>
#endif
simdutf_warn_unused simdutf_constexpr23 bool| } | ||
|
|
||
| template <endianness big_endian> | ||
| inline simdutf_warn_unused simdutf_constexpr23 bool |
There was a problem hiding this comment.
this should be a simdutf really inline
Signed-off-by: Shikhar <shikharish05@gmail.com>
|
@shikharish Could you have a look at the comments ? |
|
@lemire The last commit addressed all the previous comments. |
|
@shikharish Ok, can you access the web interface at #920 and look at @pauldreik's comments. For each one of them, can you reply to Paul. It can be as simple as 'ok, I did it in my last commit', and so forth. Otherwise, we have to go through all the comments and cross-reference them with you code changes and make sure that they have been addressed. It should be far easier for you to check each comment and confirm that it was addressed, with explanations where necessary. Thanks!!! |
Closes #891