-
Notifications
You must be signed in to change notification settings - Fork 105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(c): Changed import to common/util #783
Conversation
|
c/driver/common/CMakeLists.txt
Outdated
@@ -17,8 +17,8 @@ | |||
|
|||
add_library(adbc_driver_common STATIC utils.c) | |||
set_target_properties(adbc_driver_common PROPERTIES POSITION_INDEPENDENT_CODE ON) | |||
include_directories(SYSTEM ${REPOSITORY_ROOT}) | |||
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/vendor) | |||
target_include_directories(adbc_driver_common SYSTEM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any particular reason for us to specify SYSTEM here? Just repeating what I see on other includes but not sure the purpose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want SYSTEM for this one; SYSTEM translates to -isystem
or the compiler equivalent which basically means to not issue warnings for things in that header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But for nanoarrow I wanted the SYSTEM flag for precisely that reason (that said, we could probably also include nanoarrow as non-system)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nice. Didn't realize that was the behavior with -isystem
includes
@@ -191,6 +191,9 @@ static inline void ArrowArrayStreamMove(struct ArrowArrayStream* src, | |||
#define _NANOARROW_CHECK_RANGE(x_, min_, max_) \ | |||
NANOARROW_RETURN_NOT_OK((x_ >= min_ && x_ <= max_) ? NANOARROW_OK : EINVAL) | |||
|
|||
#define _NANOARROW_CHECK_UPPER_LIMIT(x_, max_) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this was getting errors like:
/home/willayd/clones/arrow-adbc/c/vendor/nanoarrow/nanoarrow.h:192:31: error: comparison of unsigned expression in ‘>= 0’ is always true [-Werror=type-limits]
192 | NANOARROW_RETURN_NOT_OK((x_ >= min_ && x_ <= max_) ? NANOARROW_OK : EINVAL)
| ^
/home/willayd/clones/arrow-adbc/c/vendor/nanoarrow/nanoarrow.h:187:23: note: in definition of macro ‘_NANOARROW_RETURN_NOT_OK_IMPL’
187 | const int NAME = (EXPR); \
| ^~~~
/home/willayd/clones/arrow-adbc/c/vendor/nanoarrow/nanoarrow.h:192:3: note: in expansion of macro ‘NANOARROW_RETURN_NOT_OK’
192 | NANOARROW_RETURN_NOT_OK((x_ >= min_ && x_ <= max_) ? NANOARROW_OK : EINVAL)
| ^~~~~~~~~~~~~~~~~~~~~~~
/home/willayd/clones/arrow-adbc/c/vendor/nanoarrow/nanoarrow.h:2606:7: note: in expansion of macro ‘_NANOARROW_CHECK_RANGE’
2606 | _NANOARROW_CHECK_RANGE(value, 0, UINT8_MAX);
which make sense. Not sure why they are appearing now. Need to upstream this in nanoarrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your other comment here explains why this wasn't producing warnings/errors before
No description provided.