You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "WillAyd (via GitHub)" <gi...@apache.org> on 2023/04/14 18:20:25 UTC
[GitHub] [arrow-adbc] WillAyd opened a new pull request, #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
WillAyd opened a new pull request, #587:
URL: https://github.com/apache/arrow-adbc/pull/587
Cuts down on the verbosity of string appends and bridges the functional gap between this and the templated C++ version a bit.
In follow ups would be nice to support non-string arguments, clear up the error handling, and make the Append function non-void with a return value
--
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-adbc] WillAyd commented on a diff in pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#discussion_r1167290657
##########
c/driver/common/utils.c:
##########
@@ -124,19 +124,33 @@ void StringBuilderInit(struct StringBuilder* builder, size_t initial_size) {
builder->size = 0;
builder->capacity = initial_size;
}
-void StringBuilderAppend(struct StringBuilder* builder, const char* value) {
- size_t length = strlen(value);
- size_t new_size = builder->size + length;
- if (new_size > builder->capacity) {
- size_t new_capacity = builder->size + length - builder->capacity;
- if (builder->size == 0) new_capacity++;
-
- builder->buffer = realloc(builder->buffer, new_capacity);
- builder->capacity = new_capacity;
+void StringBuilderAppend(struct StringBuilder* builder, const char* fmt, ...) {
+ va_list argptr;
+ char* value;
+
+ va_start(argptr, fmt);
Review Comment:
Love this idea - never used this before. Thanks for the tip!
--
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-adbc] lidavidm commented on pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#issuecomment-1526895604
Hmm, I think we broke something on Windows at some point...
--
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-adbc] WillAyd commented on a diff in pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#discussion_r1178546977
##########
c/driver/common/utils.c:
##########
@@ -119,24 +120,41 @@ AdbcStatusCode BatchToArrayStream(struct ArrowArray* values, struct ArrowSchema*
return ADBC_STATUS_OK;
}
-void StringBuilderInit(struct StringBuilder* builder, size_t initial_size) {
+int StringBuilderInit(struct StringBuilder* builder, size_t initial_size) {
builder->buffer = (char*)malloc(initial_size);
+ if (builder->buffer == NULL) return -1;
+
builder->size = 0;
builder->capacity = initial_size;
+
+ return 0;
}
-void StringBuilderAppend(struct StringBuilder* builder, const char* value) {
- size_t length = strlen(value);
- size_t new_size = builder->size + length;
- if (new_size > builder->capacity) {
- size_t new_capacity = builder->size + length - builder->capacity;
- if (builder->size == 0) new_capacity++;
-
- builder->buffer = realloc(builder->buffer, new_capacity);
- builder->capacity = new_capacity;
+int StringBuilderAppend(struct StringBuilder* builder, const char* fmt, ...) {
+ va_list argptr;
+ int bytes_available = builder->capacity - builder->size;
+
+ va_start(argptr, fmt);
+ int n = vsnprintf(builder->buffer + builder->size, bytes_available, fmt, argptr);
+ va_end(argptr);
+
+ if (n < 0) {
+ return -1;
Review Comment:
Makes sense - will give this a look
--
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-adbc] lidavidm commented on a diff in pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#discussion_r1179873958
##########
c/driver/common/utils.c:
##########
@@ -138,11 +138,11 @@ int StringBuilderAppend(struct StringBuilder* builder, const char* fmt, ...) {
va_end(argptr);
if (n < 0) {
- return -1;
+ return errno;
Review Comment:
Hmm, my manpage on macOS doesn't mention errno at all.
Ah, but it does mention specific _codes_ without explicitly mentioning errno. OK, this is fine then. Sorry!
--
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-adbc] WillAyd commented on pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#issuecomment-1522756940
I think the current CI failures are unrelated - at least seeing them on other PRs
--
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-adbc] lidavidm commented on pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#issuecomment-1526896206
#597 post-merge shows the same Windows error oddly
--
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-adbc] paleolimbot commented on a diff in pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#discussion_r1167221556
##########
c/driver/common/utils.c:
##########
@@ -124,19 +124,33 @@ void StringBuilderInit(struct StringBuilder* builder, size_t initial_size) {
builder->size = 0;
builder->capacity = initial_size;
}
-void StringBuilderAppend(struct StringBuilder* builder, const char* value) {
- size_t length = strlen(value);
- size_t new_size = builder->size + length;
- if (new_size > builder->capacity) {
- size_t new_capacity = builder->size + length - builder->capacity;
- if (builder->size == 0) new_capacity++;
-
- builder->buffer = realloc(builder->buffer, new_capacity);
- builder->capacity = new_capacity;
+void StringBuilderAppend(struct StringBuilder* builder, const char* fmt, ...) {
+ va_list argptr;
+ char* value;
+
+ va_start(argptr, fmt);
Review Comment:
Can you use `vsnprintf()` here and nanoarrow's `ArrowBuffer` to simplify anything here? Maybe:
```c
int chars_required = vsnprintf(NULL, 0, fmt, va_args);
ArrowBufferReserve(buffer, chars_required + 1)
vsnprintf(buffer->data + buffer->size_bytes, chars_required, fmt, va_args);
buffer->size_bytes += chars_required;
```
(Maybe I'm misunderstanding what this does too though...also I forget the exact details of where you have to do + 1 with the return value of vsnprintf)
--
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-adbc] WillAyd commented on a diff in pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#discussion_r1179855463
##########
c/driver/common/utils.c:
##########
@@ -138,11 +138,11 @@ int StringBuilderAppend(struct StringBuilder* builder, const char* fmt, ...) {
va_end(argptr);
if (n < 0) {
- return -1;
+ return errno;
Review Comment:
Hmm really? Trying to find a definitive list of library calls that do or don't set errno without luck, but as far as I can tell in POSIX (through clicking all the references in https://pubs.opengroup.org/onlinepubs/7908799/xsh/vsprintf.html) this has similar error handling to fputc which does explicitly set errno https://pubs.opengroup.org/onlinepubs/7908799/xsh/fputc.html
Microsoft documentation is thankfully more explicit:
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/vsnprintf-vsnprintf-vsnprintf-l-vsnwprintf-vsnwprintf-l?view=msvc-170#return-value
--
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-adbc] lidavidm commented on a diff in pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#discussion_r1179847811
##########
c/driver/common/utils.c:
##########
@@ -138,11 +138,11 @@ int StringBuilderAppend(struct StringBuilder* builder, const char* fmt, ...) {
va_end(argptr);
if (n < 0) {
- return -1;
+ return errno;
Review Comment:
From man page, vsnprintf won't set errno so we should return our own value (EIO?)
--
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-adbc] lidavidm commented on a diff in pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#discussion_r1178546566
##########
c/driver/common/utils.c:
##########
@@ -119,24 +120,41 @@ AdbcStatusCode BatchToArrayStream(struct ArrowArray* values, struct ArrowSchema*
return ADBC_STATUS_OK;
}
-void StringBuilderInit(struct StringBuilder* builder, size_t initial_size) {
+int StringBuilderInit(struct StringBuilder* builder, size_t initial_size) {
builder->buffer = (char*)malloc(initial_size);
+ if (builder->buffer == NULL) return -1;
+
builder->size = 0;
builder->capacity = initial_size;
+
+ return 0;
}
-void StringBuilderAppend(struct StringBuilder* builder, const char* value) {
- size_t length = strlen(value);
- size_t new_size = builder->size + length;
- if (new_size > builder->capacity) {
- size_t new_capacity = builder->size + length - builder->capacity;
- if (builder->size == 0) new_capacity++;
-
- builder->buffer = realloc(builder->buffer, new_capacity);
- builder->capacity = new_capacity;
+int StringBuilderAppend(struct StringBuilder* builder, const char* fmt, ...) {
+ va_list argptr;
+ int bytes_available = builder->capacity - builder->size;
+
+ va_start(argptr, fmt);
+ int n = vsnprintf(builder->buffer + builder->size, bytes_available, fmt, argptr);
+ va_end(argptr);
+
+ if (n < 0) {
+ return -1;
Review Comment:
in general, maybe return one of the errno codes for consistency with the nanoarrow behavior?
--
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-adbc] WillAyd commented on pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#issuecomment-1511769921
Added a test module for this, although I'm not sure where to actually build that test module in the current CMake setup. It may be time to reconsider https://github.com/apache/arrow-adbc/pull/314 (N.B. - not a build system expert by any means)
--
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-adbc] WillAyd commented on a diff in pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#discussion_r1178015472
##########
c/driver/common/utils.c:
##########
@@ -124,19 +124,27 @@ void StringBuilderInit(struct StringBuilder* builder, size_t initial_size) {
builder->size = 0;
builder->capacity = initial_size;
}
-void StringBuilderAppend(struct StringBuilder* builder, const char* value) {
- size_t length = strlen(value);
- size_t new_size = builder->size + length;
- if (new_size > builder->capacity) {
- size_t new_capacity = builder->size + length - builder->capacity;
- if (builder->size == 0) new_capacity++;
-
- builder->buffer = realloc(builder->buffer, new_capacity);
- builder->capacity = new_capacity;
+void StringBuilderAppend(struct StringBuilder* builder, const char* fmt, ...) {
+ va_list argptr;
+ int bytes_available = builder->capacity - builder->size;
+
+ va_start(argptr, fmt);
+ int n = vsnprintf(builder->buffer + builder->size, bytes_available, fmt, argptr);
+ va_end(argptr);
+
+ if (n < 0) { // TODO: handle error
Review Comment:
Yea we should. Ideally these aren't void functions but return an integer to indicate success / failure.
Without trying to do too much at once I am thinking of that as a follow up, but can try to tackle within this too if it doesn't make the diff too large
--
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-adbc] lidavidm commented on pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#issuecomment-1526896876
I'll have to look at it when I can get a Windows VM again but I suppose let's merge this for the time being
Filed #616
--
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-adbc] WillAyd commented on a diff in pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#discussion_r1167246239
##########
c/driver/common/utils.c:
##########
@@ -124,19 +124,33 @@ void StringBuilderInit(struct StringBuilder* builder, size_t initial_size) {
builder->size = 0;
builder->capacity = initial_size;
}
-void StringBuilderAppend(struct StringBuilder* builder, const char* value) {
- size_t length = strlen(value);
- size_t new_size = builder->size + length;
- if (new_size > builder->capacity) {
- size_t new_capacity = builder->size + length - builder->capacity;
- if (builder->size == 0) new_capacity++;
-
- builder->buffer = realloc(builder->buffer, new_capacity);
- builder->capacity = new_capacity;
+void StringBuilderAppend(struct StringBuilder* builder, const char* fmt, ...) {
+ va_list argptr;
+ char* value;
+
+ va_start(argptr, fmt);
Review Comment:
Nice idea - let me try that
--
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-adbc] lidavidm commented on pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#issuecomment-1512230389
> Added a test module for this, although I'm not sure where to actually build that test module in the current CMake setup. It may be time to reconsider #314 (N.B. - not a build system expert by any means)
Yeah, I suppose we can't avoid it anymore unfortunately.
--
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-adbc] WillAyd commented on a diff in pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#discussion_r1167294994
##########
c/driver/common/utils.c:
##########
@@ -124,19 +124,26 @@ void StringBuilderInit(struct StringBuilder* builder, size_t initial_size) {
builder->size = 0;
builder->capacity = initial_size;
}
-void StringBuilderAppend(struct StringBuilder* builder, const char* value) {
- size_t length = strlen(value);
- size_t new_size = builder->size + length;
- if (new_size > builder->capacity) {
- size_t new_capacity = builder->size + length - builder->capacity;
- if (builder->size == 0) new_capacity++;
-
- builder->buffer = realloc(builder->buffer, new_capacity);
- builder->capacity = new_capacity;
+void StringBuilderAppend(struct StringBuilder* builder, const char* fmt, ...) {
+ va_list argptr;
+ size_t bytes_available = builder->capacity - builder->size;
+
+ va_start(argptr, fmt);
+ int n = vsnprintf(builder->buffer + builder->size, bytes_available, fmt, argptr);
+ va_end(argptr);
+
+ if (n < 0) { // TODO: handle error
+ } else if (n >= bytes_available) { // output was truncated
+ builder->buffer = (char*)realloc(builder->buffer, builder->capacity + n + 1);
Review Comment:
Might be misreading the current implementation but I think it still stores the null terminator, just doesn't include it as part of size. Doing the same here, hence the `n + 1` in allocation / capacity expressions but just `+= n` for the size
--
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-adbc] lidavidm commented on a diff in pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#discussion_r1177418073
##########
c/driver/common/utils.c:
##########
@@ -28,7 +28,7 @@
static size_t kErrorBufferSize = 256;
static char kErrorPrefix[] = "[SQLite] ";
-void ReleaseError(struct AdbcError* error) {
+static void ReleaseError(struct AdbcError* error) {
Review Comment:
Hmm, were these being multiply defined somehow?
##########
c/driver/common/utils.c:
##########
@@ -124,19 +124,27 @@ void StringBuilderInit(struct StringBuilder* builder, size_t initial_size) {
builder->size = 0;
builder->capacity = initial_size;
}
-void StringBuilderAppend(struct StringBuilder* builder, const char* value) {
- size_t length = strlen(value);
- size_t new_size = builder->size + length;
- if (new_size > builder->capacity) {
- size_t new_capacity = builder->size + length - builder->capacity;
- if (builder->size == 0) new_capacity++;
-
- builder->buffer = realloc(builder->buffer, new_capacity);
- builder->capacity = new_capacity;
+void StringBuilderAppend(struct StringBuilder* builder, const char* fmt, ...) {
+ va_list argptr;
+ int bytes_available = builder->capacity - builder->size;
+
+ va_start(argptr, fmt);
+ int n = vsnprintf(builder->buffer + builder->size, bytes_available, fmt, argptr);
+ va_end(argptr);
+
+ if (n < 0) { // TODO: handle error
Review Comment:
Should we handle the TODO here?
##########
c/driver/common/utils.c:
##########
@@ -124,19 +124,27 @@ void StringBuilderInit(struct StringBuilder* builder, size_t initial_size) {
builder->size = 0;
builder->capacity = initial_size;
}
-void StringBuilderAppend(struct StringBuilder* builder, const char* value) {
- size_t length = strlen(value);
- size_t new_size = builder->size + length;
- if (new_size > builder->capacity) {
- size_t new_capacity = builder->size + length - builder->capacity;
- if (builder->size == 0) new_capacity++;
-
- builder->buffer = realloc(builder->buffer, new_capacity);
- builder->capacity = new_capacity;
+void StringBuilderAppend(struct StringBuilder* builder, const char* fmt, ...) {
+ va_list argptr;
+ int bytes_available = builder->capacity - builder->size;
+
+ va_start(argptr, fmt);
+ int n = vsnprintf(builder->buffer + builder->size, bytes_available, fmt, argptr);
+ va_end(argptr);
+
+ if (n < 0) { // TODO: handle error
+ } else if (n >= bytes_available) { // output was truncated
+ int bytes_needed = n - bytes_available + 1;
+ builder->buffer = (char*)realloc(builder->buffer, builder->capacity + bytes_needed);
+ if (builder->buffer == NULL) { /* TODO: handle error */
+ }
+ builder->capacity += bytes_needed;
+
+ va_start(argptr, fmt);
+ vsnprintf(builder->buffer + builder->size, n + 1, fmt, argptr);
Review Comment:
possibly, assert on the return value?
##########
c/driver/common/utils.h:
##########
@@ -44,7 +48,7 @@ struct StringBuilder {
size_t capacity;
};
void StringBuilderInit(struct StringBuilder* builder, size_t initial_size);
-void StringBuilderAppend(struct StringBuilder* builder, const char* value);
+void StringBuilderAppend(struct StringBuilder* builder, const char* fmt, ...);
Review Comment:
IIRC, there's a compiler-specific attribute you can apply to make the compiler apply warnings like it does for printf & friends
##########
c/driver/common/utils_test.cc:
##########
@@ -0,0 +1,62 @@
+// 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.
+
+#include <gtest/gtest.h>
+
+#include "utils.h"
+
+TEST(TestStringBuilder, TestBasic) {
+ struct StringBuilder str;
+ StringBuilderInit(&str, /*initial_size=*/64);
+ EXPECT_EQ(str.capacity, 64);
+ StringBuilderAppend(&str, "%s", "BASIC TEST");
+ EXPECT_EQ(str.size, 10);
+ EXPECT_STREQ(str.buffer, "BASIC TEST");
+
+ StringBuilderReset(&str);
+}
+
+TEST(TestStringBuilder, TestBoundary) {
+ struct StringBuilder str;
+ StringBuilderInit(&str, /*initial_size=*/10);
+ EXPECT_EQ(str.capacity, 10);
+ StringBuilderAppend(&str, "%s", "BASIC TEST");
+ // should resize to include \n
Review Comment:
is that the \n or the \0?
--
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-adbc] WillAyd commented on a diff in pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#discussion_r1178010942
##########
c/driver/common/utils_test.cc:
##########
@@ -0,0 +1,62 @@
+// 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.
+
+#include <gtest/gtest.h>
+
+#include "utils.h"
+
+TEST(TestStringBuilder, TestBasic) {
+ struct StringBuilder str;
+ StringBuilderInit(&str, /*initial_size=*/64);
+ EXPECT_EQ(str.capacity, 64);
+ StringBuilderAppend(&str, "%s", "BASIC TEST");
+ EXPECT_EQ(str.size, 10);
+ EXPECT_STREQ(str.buffer, "BASIC TEST");
+
+ StringBuilderReset(&str);
+}
+
+TEST(TestStringBuilder, TestBoundary) {
+ struct StringBuilder str;
+ StringBuilderInit(&str, /*initial_size=*/10);
+ EXPECT_EQ(str.capacity, 10);
+ StringBuilderAppend(&str, "%s", "BASIC TEST");
+ // should resize to include \n
Review Comment:
You are correct - typo
--
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-adbc] WillAyd commented on a diff in pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#discussion_r1178013337
##########
c/driver/common/utils.c:
##########
@@ -28,7 +28,7 @@
static size_t kErrorBufferSize = 256;
static char kErrorPrefix[] = "[SQLite] ";
-void ReleaseError(struct AdbcError* error) {
+static void ReleaseError(struct AdbcError* error) {
Review Comment:
Don't think it's an immediate issue but since these aren't exposed in the header I think safer to keep them scoped to only usable within the TU
--
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-adbc] lidavidm commented on a diff in pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587#discussion_r1178545188
##########
c/driver/common/utils.c:
##########
@@ -124,19 +124,27 @@ void StringBuilderInit(struct StringBuilder* builder, size_t initial_size) {
builder->size = 0;
builder->capacity = initial_size;
}
-void StringBuilderAppend(struct StringBuilder* builder, const char* value) {
- size_t length = strlen(value);
- size_t new_size = builder->size + length;
- if (new_size > builder->capacity) {
- size_t new_capacity = builder->size + length - builder->capacity;
- if (builder->size == 0) new_capacity++;
-
- builder->buffer = realloc(builder->buffer, new_capacity);
- builder->capacity = new_capacity;
+void StringBuilderAppend(struct StringBuilder* builder, const char* fmt, ...) {
+ va_list argptr;
+ int bytes_available = builder->capacity - builder->size;
+
+ va_start(argptr, fmt);
+ int n = vsnprintf(builder->buffer + builder->size, bytes_available, fmt, argptr);
+ va_end(argptr);
+
+ if (n < 0) { // TODO: handle error
Review Comment:
A follow up is fine (just make sure to file it so it can be `TODO(...)`
--
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-adbc] lidavidm merged pull request #587: refactor(c/driver/common): Variadic arguments for StringBuilderAppend
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #587:
URL: https://github.com/apache/arrow-adbc/pull/587
--
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