mkmkme commented on code in PR #2966:
URL: https://github.com/apache/avro/pull/2966#discussion_r1677524359
##########
lang/c++/impl/Node.cc:
##########
@@ -146,7 +146,7 @@ void Node::setLogicalType(LogicalType logicalType) {
if (type_ == AVRO_FIXED) {
// Max precision that can be supported by the current size of
// the FIXED type.
- long maxPrecision = floor(log10(2.0) * (8.0 * fixedSize() -
1));
+ int32_t maxPrecision = static_cast<int32_t>(floor(log10(2.0) *
(8.0 * static_cast<double>(fixedSize()) - 1)));
Review Comment:
Here definitely `auto`, no need to type the type twice
##########
lang/c++/impl/NodeImpl.cc:
##########
@@ -577,9 +577,9 @@ NodeMap::NodeMap() : NodeImplMap(AVRO_MAP) {
void NodeUnion::printJson(std::ostream &os, size_t depth) const {
os << "[\n";
- int fields = leafAttributes_.size();
+ size_t fields = leafAttributes_.size();
Review Comment:
ditto
##########
lang/c++/test/buffertest.cc:
##########
@@ -34,19 +34,18 @@ using detail::kMinBlockSize;
using std::cout;
using std::endl;
+// Make a string of repeating 0123456789ABCDEF0123456789...
std::string makeString(size_t len) {
- std::string newstring;
- newstring.reserve(len);
+ std::string result;
+ result.reserve(len);
+
+ constexpr auto chars = "0123456789ABCDEF";
Review Comment:
Not sure how `auto chars` will be resolved in this case, but I'd declare it
as `constexpr auto chars[]` so that it would be definitely considered an array
and any possible out-of-bounds `constexpr` access would be caught
##########
lang/c++/include/avro/Reader.hh:
##########
@@ -176,15 +176,15 @@ private:
return encoded;
}
- int64_t readSize() {
+ size_t readSize() {
Review Comment:
This one looks a bit scary to me. It introduces a breaking change by
changing the sign-ness of the return value. If not careful, it can be
dangerously misrepresented on the user side.
##########
lang/c++/impl/FileStream.cc:
##########
@@ -94,7 +94,7 @@ struct FileBufferCopyIn : public BufferCopyIn {
}
bool read(uint8_t *b, size_t toRead, size_t &actual) final {
- int n = ::read(fd_, b, toRead);
+ ssize_t n = ::read(fd_, b, toRead);
Review Comment:
`auto`?
##########
lang/c++/include/avro/buffer/BufferStreambuf.hh:
##########
@@ -135,7 +135,11 @@ protected:
memcpy(c, gptr(), toCopy);
c += toCopy;
bytesCopied += toCopy;
- gbump(toCopy);
+ while (toCopy > INT_MAX) {
Review Comment:
I'd prefer using
[numeric_limits](https://en.cppreference.com/w/cpp/types/numeric_limits/max)
here as using macros can be fragile.
##########
lang/c++/include/avro/Validator.hh:
##########
@@ -35,7 +35,7 @@ public:
explicit NullValidator(const ValidSchema &) {}
Review Comment:
This file also introduces user-facing sign-ness changes
##########
lang/c++/impl/NodeImpl.cc:
##########
@@ -531,9 +531,9 @@ void NodeEnum::printJson(std::ostream &os, size_t depth)
const {
printName(os, nameAttribute_.get(), depth);
os << indent(depth) << "\"symbols\": [\n";
- int names = leafNameAttributes_.size();
+ size_t names = leafNameAttributes_.size();
Review Comment:
`auto` please
##########
lang/c++/impl/parsing/ResolvingDecoder.cc:
##########
@@ -90,8 +91,8 @@ Symbol ResolvingGrammarGenerator::generate(
return Symbol::rootSymbol(main, backup);
}
-int ResolvingGrammarGenerator::bestBranch(const NodePtr &writer,
- const NodePtr &reader) {
+std::optional<size_t> ResolvingGrammarGenerator::bestBranch(const NodePtr
&writer,
Review Comment:
That's a really good one, thanks!
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
