szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596888000
##########
File path: extensions/script/lua/LuaBaseStream.cpp
##########
@@ -52,16 +52,11 @@ std::string LuaBaseStream::read(size_t len) {
// 0 <= n < s.size()."
//
// http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf
- auto read = stream_->read(reinterpret_cast<uint8_t *>(&buffer[0]),
static_cast<int>(len));
- if (read < 0) {
- return nullptr;
- }
-
- if (gsl::narrow<size_t>(read) != len) {
- buffer.resize(gsl::narrow<size_t>(read));
+ const auto read = stream_->read(reinterpret_cast<uint8_t *>(&buffer[0]),
len);
+ if (!io::isError(read) && read != len) {
+ buffer.resize(read);
}
-
- return buffer;
+ return io::isError(read) ? std::string{} : buffer;
Review comment:
I didn't know that libstdc++ throws there. Despite being an unrelated
change, I really don't want to rely on undefined behavior there, so I insist on
a change.
In the old main version (that this PR was originally based on), there was no
string construction from nullptr here (but the one on line 42 was already
present). I believe that change on main to be an error, so by moving to
returning an empty string, we basically revert to the old, correct version, the
only change being that we no longer return an empty string of size `len`.
If you insist on throwing, I can change this to throw something explicitly,
but I prefer the old behavior of returning an empty string. I want to change
the occurrence on line 42 to be consistent with this one, too.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
