Copilot commented on code in PR #61368:
URL: https://github.com/apache/doris/pull/61368#discussion_r2938273042
##########
thirdparty/build-thirdparty.sh:
##########
@@ -551,6 +551,13 @@ build_snappy() {
check_if_source_exist "${SNAPPY_SOURCE}"
cd "${TP_SOURCE_DIR}/${SNAPPY_SOURCE}"
+ # Enable RTTI for snappy (required by Doris BE for SnappySlicesSource
inheritance)
+ if [[ "${KERNEL}" == 'Darwin' ]]; then
+ sed -i '' 's/-fno-rtti/-frtti/g' CMakeLists.txt
+ else
+ sed -i 's/-fno-rtti/-frtti/g' CMakeLists.txt
+ fi
Review Comment:
The comment says RTTI is required for `SnappySlicesSource` inheritance, but
inheriting from `snappy::Source` doesn’t require RTTI; RTTI is only needed for
`dynamic_cast`/`typeid` on polymorphic types. In the BE codebase,
`SnappySlicesSource` is only used via virtual dispatch (no `dynamic_cast` on
snappy types), so this rationale looks incorrect/misleading. Please update the
comment to reflect the actual reason RTTI must be enabled (if there is one), or
remove the claim about inheritance.
##########
thirdparty/download-thirdparty.sh:
##########
@@ -269,6 +269,17 @@ if [[ " ${TP_ARCHIVES[*]} " =~ " GLOG " ]]; then
echo "Finished patching ${GLOG_SOURCE}"
fi
+# snappy patch to fix sign-compare warning
+if [[ " ${TP_ARCHIVES[*]} " =~ " SNAPPY " ]]; then
+ cd "${TP_SOURCE_DIR}/${SNAPPY_SOURCE}"
+ if [[ ! -f "${PATCHED_MARK}" ]]; then
+ patch -p1 <"${TP_PATCH_DIR}/snappy-1.1.10-sign-compare.patch"
+ touch "${PATCHED_MARK}"
+ fi
+ cd -
+ echo "Finished patching ${SNAPPY_SOURCE}"
Review Comment:
The snappy patch being applied is version-specific
(`snappy-1.1.10-sign-compare.patch`), but this block doesn’t guard on
`SNAPPY_SOURCE` like other patch blocks (e.g., glog). If `SNAPPY_SOURCE`
changes in the future, this will try to apply the wrong patch and fail the
thirdparty download step. Add a `SNAPPY_SOURCE == snappy-1.1.10` (or similar)
condition around the patch application, or derive the patch filename from the
version in a controlled way.
--
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]
