You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2023/05/08 15:16:00 UTC
[GitHub] [arrow-nanoarrow] paleolimbot opened a new pull request, #189: Add more C++ and error handling helpers
paleolimbot opened a new pull request, #189:
URL: https://github.com/apache/arrow-nanoarrow/pull/189
(no comment)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] codecov-commenter commented on pull request #189: Add more C++ and error handling helpers
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #189:
URL: https://github.com/apache/arrow-nanoarrow/pull/189#issuecomment-1538582320
## [Codecov](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/189?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#189](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/189?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a37e722) into [main](https://app.codecov.io/gh/apache/arrow-nanoarrow/commit/9b034ad916eceec4e9d8300633d0b65687d25511?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9b034ad) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## main #189 +/- ##
=======================================
Coverage 87.72% 87.73%
=======================================
Files 57 58 +1
Lines 8783 8786 +3
=======================================
+ Hits 7705 7708 +3
Misses 1078 1078
```
| [Impacted Files](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/189?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [src/nanoarrow/nanoarrow\_types.h](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/189?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL25hbm9hcnJvdy9uYW5vYXJyb3dfdHlwZXMuaA==) | `92.75% <ø> (ø)` | |
| [src/nanoarrow/nanoarrow.h](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/189?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL25hbm9hcnJvdy9uYW5vYXJyb3cuaA==) | `100.00% <100.00%> (ø)` | |
| [src/nanoarrow/nanoarrow.hpp](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/189?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL25hbm9hcnJvdy9uYW5vYXJyb3cuaHBw) | `98.05% <100.00%> (+0.03%)` | :arrow_up: |
... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/189/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] paleolimbot commented on a diff in pull request #189: Add more C++ and error handling helpers
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #189:
URL: https://github.com/apache/arrow-nanoarrow/pull/189#discussion_r1188565868
##########
CMakeLists.txt:
##########
@@ -41,6 +41,12 @@ else()
set(NANOARROW_NAMESPACE_DEFINE "// #define NANOARROW_NAMESPACE YourNamespaceHere")
endif()
+if (CMAKE_BUILD_TYPE STREQUAL "Debug")
+ set(NANOARROW_DEBUG_DEFINE "#define NANOARROW_DEBUG 1")
+else()
+ set(NANOARROW_DEBUG_DEFINE "// #define NANOARROW_DEBUG 1")
+endif()
Review Comment:
Thanks for this...I like the `target_compile_definitions()` solution better. Debug mode should probably never be hard-coded in a header and should probably always be invoked via a compiler define.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] kou commented on a diff in pull request #189: Add more C++ and error handling helpers
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #189:
URL: https://github.com/apache/arrow-nanoarrow/pull/189#discussion_r1188579876
##########
CMakeLists.txt:
##########
@@ -41,6 +41,12 @@ else()
set(NANOARROW_NAMESPACE_DEFINE "// #define NANOARROW_NAMESPACE YourNamespaceHere")
endif()
+if (CMAKE_BUILD_TYPE STREQUAL "Debug")
+ set(NANOARROW_DEBUG_DEFINE "#define NANOARROW_DEBUG 1")
+else()
+ set(NANOARROW_DEBUG_DEFINE "// #define NANOARROW_DEBUG 1")
+endif()
Review Comment:
We may want to use generator expression: https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html
```cmake
# We don't need to put this under if(CMAKE_BUILD_TYPE STREQUAL "Debug")
target_compile_definitions(nanoarrow PUBLIC "$<$<CONFIG:Debug>:NANOARROW_DEBUG>")
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] lidavidm commented on a diff in pull request #189: Add more C++ and error handling helpers
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #189:
URL: https://github.com/apache/arrow-nanoarrow/pull/189#discussion_r1188532573
##########
CMakeLists.txt:
##########
@@ -41,6 +41,12 @@ else()
set(NANOARROW_NAMESPACE_DEFINE "// #define NANOARROW_NAMESPACE YourNamespaceHere")
endif()
+if (CMAKE_BUILD_TYPE STREQUAL "Debug")
+ set(NANOARROW_DEBUG_DEFINE "#define NANOARROW_DEBUG 1")
+else()
+ set(NANOARROW_DEBUG_DEFINE "// #define NANOARROW_DEBUG 1")
+endif()
Review Comment:
FWIW, CMake has an explicit feature for this: [target_compile_definitions](https://cmake.org/cmake/help/latest/command/target_compile_definitions.html)
##########
CMakeLists.txt:
##########
@@ -41,6 +41,12 @@ else()
set(NANOARROW_NAMESPACE_DEFINE "// #define NANOARROW_NAMESPACE YourNamespaceHere")
endif()
+if (CMAKE_BUILD_TYPE STREQUAL "Debug")
+ set(NANOARROW_DEBUG_DEFINE "#define NANOARROW_DEBUG 1")
+else()
+ set(NANOARROW_DEBUG_DEFINE "// #define NANOARROW_DEBUG 1")
+endif()
Review Comment:
Ah, though I suppose this is different since it'll propagate to anyone who includes nanoarrow_config.h.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] paleolimbot merged pull request #189: Add more C++ and error handling helpers
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #189:
URL: https://github.com/apache/arrow-nanoarrow/pull/189
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] paleolimbot commented on a diff in pull request #189: Add more C++ and error handling helpers
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #189:
URL: https://github.com/apache/arrow-nanoarrow/pull/189#discussion_r1188587916
##########
CMakeLists.txt:
##########
@@ -41,6 +41,12 @@ else()
set(NANOARROW_NAMESPACE_DEFINE "// #define NANOARROW_NAMESPACE YourNamespaceHere")
endif()
+if (CMAKE_BUILD_TYPE STREQUAL "Debug")
+ set(NANOARROW_DEBUG_DEFINE "#define NANOARROW_DEBUG 1")
+else()
+ set(NANOARROW_DEBUG_DEFINE "// #define NANOARROW_DEBUG 1")
+endif()
Review Comment:
Thanks! That is much cleaner.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org