kou commented on code in PR #2738: URL: https://github.com/apache/arrow-adbc/pull/2738#discussion_r2064961480
########## c/include/arrow-adbc/driver/bigquery.h: ########## @@ -0,0 +1,35 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +/// \file arrow-adbc/driver/bigquery.h ADBC BigQuery Driver +/// +/// A driver for BigQuery. + +#pragma once + +#include <arrow-adbc/adbc.h> + +#ifdef __cplusplus +extern "C" { +#endif + +ADBC_EXPORT +AdbcStatusCode BigQueryDriverInit(int version, void* raw_driver, struct AdbcError* error); Review Comment: We may want to add `Adbc` prefix for `*DriverInit()` functions: ```suggestion AdbcStatusCode AdbcBigQueryDriverInit(int version, void* raw_driver, struct AdbcError* error); ``` ########## c/cmake_modules/GoUtils.cmake: ########## @@ -135,10 +135,20 @@ function(add_go_lib GO_MOD_DIR GO_LIBNAME) "${GO_BUILD_FLAGS} $<$<CONFIG:DEBUG>:-gcflags=\"-N -l\">") # if we're building debug mode then change the default CGO_CFLAGS and CGO_CXXFLAGS from "-g O2" to "-g3" - set(GO_ENV_VARS - "CGO_ENABLED=1 $<$<CONFIG:DEBUG>:CGO_CFLAGS=-g3> $<$<CONFIG:DEBUG>:CGO_CXXFLAGS=-g3>" - ) - separate_arguments(GO_ENV_VARS NATIVE_COMMAND "${GO_ENV_VARS}") + set(GO_FLAGS) + if(CMAKE_BUILD_TYPE STREQUAL "Debug") + set(GO_FLAGS "-g3") + endif() + string(JOIN " " GO_FLAGS "${GO_FLAGS}") + + foreach(DEFINE ${ARG_DEFINES}) + list(APPEND GO_FLAGS "-D${DEFINE}") Review Comment: It seems that `GO_FLAGS` is string here. Should we use `string(APPEND)` here? ```suggestion string(JOIN " " GO_FLAGS "${GO_FLAGS}") foreach(DEFINE ${ARG_DEFINES}) string(APPEND GO_FLAGS " -D${DEFINE}") ``` ########## c/cmake_modules/GoUtils.cmake: ########## @@ -135,10 +135,20 @@ function(add_go_lib GO_MOD_DIR GO_LIBNAME) "${GO_BUILD_FLAGS} $<$<CONFIG:DEBUG>:-gcflags=\"-N -l\">") # if we're building debug mode then change the default CGO_CFLAGS and CGO_CXXFLAGS from "-g O2" to "-g3" - set(GO_ENV_VARS - "CGO_ENABLED=1 $<$<CONFIG:DEBUG>:CGO_CFLAGS=-g3> $<$<CONFIG:DEBUG>:CGO_CXXFLAGS=-g3>" - ) - separate_arguments(GO_ENV_VARS NATIVE_COMMAND "${GO_ENV_VARS}") + set(GO_FLAGS) + if(CMAKE_BUILD_TYPE STREQUAL "Debug") + set(GO_FLAGS "-g3") + endif() + string(JOIN " " GO_FLAGS "${GO_FLAGS}") + + foreach(DEFINE ${ARG_DEFINES}) + list(APPEND GO_FLAGS "-D${DEFINE}") + endforeach() + + set(GO_ENV_VARS) + list(APPEND GO_ENV_VARS "CGO_ENABLED=1") + list(APPEND GO_ENV_VARS "CGO_CFLAGS=\"${GO_FLAGS}\"") + list(APPEND GO_ENV_VARS "CGO_CXXFLAGS=\"${GO_FLAGS}\"") Review Comment: Do we need to escape `"` here? (Does `cmake -E env` care about it?) ########## c/cmake_modules/GoUtils.cmake: ########## @@ -135,10 +135,20 @@ function(add_go_lib GO_MOD_DIR GO_LIBNAME) "${GO_BUILD_FLAGS} $<$<CONFIG:DEBUG>:-gcflags=\"-N -l\">") # if we're building debug mode then change the default CGO_CFLAGS and CGO_CXXFLAGS from "-g O2" to "-g3" - set(GO_ENV_VARS - "CGO_ENABLED=1 $<$<CONFIG:DEBUG>:CGO_CFLAGS=-g3> $<$<CONFIG:DEBUG>:CGO_CXXFLAGS=-g3>" - ) - separate_arguments(GO_ENV_VARS NATIVE_COMMAND "${GO_ENV_VARS}") + set(GO_FLAGS) + if(CMAKE_BUILD_TYPE STREQUAL "Debug") + set(GO_FLAGS "-g3") + endif() Review Comment: We can use generator expression: ```suggestion set(GO_FLAGS "$<$<CONFIG:Debug>:-g3>") ``` In general generator expression is better than `CMAKE_BUILD_TYPE STREQUAL "Debug"` because `STREQUAL` is case sensitive but `$<CONFIG:Debug>` is case insensitive (`CMAKE_BUILD_TYPE=DEBUG` is also accepted like other `CMAKE_BUILD_TYPE` checks). -- 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: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
