Felix-Gong commented on PR #3390: URL: https://github.com/apache/brpc/pull/3390#issuecomment-4999566207
## Review Comments Response ### 1. BUILD.bazel: `modp_b64_rvv.cc` does not exist > `BUTIL_SRCS` includes `src/butil/third_party/modp_b64/modp_b64_rvv.cc`, but that file does not exist in the repo. **Fixed.** Removed `modp_b64_rvv.cc` from `BUTIL_SRCS` — it was inadvertently included from a prior PR (#3388) and does not belong in this change. ### 2. string_piece.h: Missing `BUTIL_EXPORT` > `rvv_memcmp`/`rvv_memchr` are declared in a public header but not marked with `BUTIL_EXPORT`. **Fixed.** Added `BUTIL_EXPORT` to the `rvv_memcmp` declaration to ensure proper symbol visibility in shared library builds. ### 3. string_compare_rvv.cc: Return value semantics > `rvv_memcmp` returns only -1 or 1 on mismatch, which may change observable behavior vs `memcmp`. **Fixed.** Changed to `return static_cast<int>(src1[first]) - static_cast<int>(src2[first]);` which matches glibc `memcmp` semantics and preserves the byte-difference contract expected by `traits_type::compare`. ### 4. string_compare_rvv.cc: NULL vs nullptr > The doc comment says this returns `NULL` when not found, but the implementation returns `nullptr`. **Addressed.** `rvv_memchr` has been removed from this PR and will be added in a follow-up PR with proper `StringPiece::find` dispatch. The doc comment and implementation will be aligned in that PR. ### 5. rvv_memchr has no call sites > This PR adds an RVV `rvv_memchr`, but there are no call sites. Consider dropping `rvv_memchr` to avoid carrying unused code. **Agreed.** `rvv_memchr` has been removed from this PR. It will be reintroduced in a follow-up PR that also wires the dispatch into `StringPiece::find(char)` via `internal::find` in `string_piece.cc`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
