De-duplicate fmt and format functions and optimize #11596
Conversation
ericcurtin
force-pushed
the
optimize_fmt
branch
2 times, most recently
from
b0a45a3 to
7435b04
Compare
|
@ngxson PTAL |
ericcurtin
force-pushed
the
optimize_fmt
branch
2 times, most recently
from
58ae4a3 to
a0c663a
Compare
src/llama-impl.cpp
Outdated
| int size2 = vsnprintf(buf.data(), size + 1, fmt, ap2); | ||
| std::string buf; | ||
| buf.resize(size); | ||
| const int size2 = vsnprintf(const_cast<char *>(buf.data()), buf.size() + 1, fmt, ap2); |
There was a problem hiding this comment.
Isn't this UB?
There was a problem hiding this comment.
As long as we don't modify past the null terminator (or .size())in the std::string, I think it's fine. At least I've done this kinda thing in various codebases for 10 years and it's never been an issue.
I think:
&buf[0]
is also an option.
But if we don't want to do this it's fine, it just seems sub-optimal to create a vector (which is essentially a poor mans std::string) and then copy all the data to a std::string.
There was a problem hiding this comment.
This looks fine for me, but I think the second argument of snprintf should be just buf.size(), without + 1
There was a problem hiding this comment.
I tested + 1 is correct, it always null terminates, we will be missing a char if we use buf.size()
There was a problem hiding this comment.
The generated string has a length of at most n-1, leaving space for the additional terminating null character.
it always prints a null character, it will truncate output if you specify too short a value you see, buf.size() is one character too short
There was a problem hiding this comment.
The + 1 is fine because the std containers do not explicitly need room for the null terminating character. This is in contrast to plain C string char buf[size] where the string length can be maximum size - 1.
Regarding the UB, I'm also sure that this will always work in the real world, but would still prefer to avoid adding UB to the code (in case this is indeed UB, not sure yet). In this case the performance consideration is not very important, because we use fmt to just print error messages and logs, so it's OK to make the copy.
There was a problem hiding this comment.
I removed the micro-optimization in case of UB. Makes the review easier for everyone.
ericcurtin
force-pushed
the
optimize_fmt
branch
from
a0c663a to
d738f18
Compare
Just use a single function, also only one buffer is required for the operation, no need to copy. Signed-off-by: Eric Curtin <[email protected]>
ericcurtin
force-pushed
the
optimize_fmt
branch
from
d738f18 to
2991954
Compare
|
@ericcurtin Has this been forgotten, or not ready to merge? |
|
We can merge if there are no complaints |
Just use a single function, also only one buffer is required for the operation, no need to copy.