thiru-mg commented on code in PR #1852:
URL: https://github.com/apache/avro/pull/1852#discussion_r996381579
##########
lang/c++/api/Validator.hh:
##########
@@ -142,7 +142,7 @@ private:
};
std::vector<CompoundType> compoundStack_;
- std::vector<size_t> counters_;
+ std::vector<int64_t> counters_;
Review Comment:
Since the counters cannot be negative, should this be `unit64_t`?
##########
lang/c++/CMakeLists.txt:
##########
@@ -64,7 +64,7 @@ if (WIN32 AND NOT CYGWIN AND NOT MSYS)
endif()
if (CMAKE_COMPILER_IS_GNUCXX)
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -pedantic -Werror")
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -pedantic -Werror
-Wconversion")
Review Comment:
Please make indentation here consistent with the rest of the file.
##########
lang/c++/impl/Compiler.cc:
##########
@@ -384,7 +396,7 @@ static NodePtr makeEnumNode(const Entity &e,
static NodePtr makeFixedNode(const Entity &e,
const Name &name, const Object &m) {
- int v = static_cast<int>(getLongField(e, m, "size"));
+ size_t v = getLongField(e, m, "size");
Review Comment:
In modern C++ we try to avoid explicitly specifying type in situations like
this: please use `auto`.
##########
lang/c++/api/buffer/BufferStreambuf.hh:
##########
@@ -130,12 +130,12 @@ protected:
size_t inBuffer = egptr() - gptr();
if (inBuffer) {
- auto remaining = static_cast<size_t>(len - bytesCopied);
- size_t toCopy = std::min(inBuffer, remaining);
+ auto remaining = len - bytesCopied;
+ size_t toCopy = std::min(inBuffer, (size_t) remaining);
memcpy(c, gptr(), toCopy);
c += toCopy;
bytesCopied += toCopy;
- gbump(toCopy);
+ gbump((int) toCopy);
Review Comment:
In C++ the intent of the cast is made explicit with `static_cast` etc. This
C style cast make it impossible to find out why is this being cast.
##########
lang/c++/impl/Zigzag.cc:
##########
@@ -30,11 +30,11 @@ encodeInt64(int64_t input, std::array<uint8_t, 10> &output)
noexcept {
auto v = val & mask;
size_t bytesOut = 0;
while (val >>= 7) {
- output[bytesOut++] = (v | 0x80);
+ output[bytesOut++] = (uint8_t) (v | 0x80);
Review Comment:
Again, please do `static_cast`.
##########
lang/c++/impl/DataFile.cc:
##########
@@ -196,10 +196,10 @@ void DataFileWriterBase::sync() {
os.push(boost::iostreams::back_inserter(temp));
boost::iostreams::write(os, compressed.c_str(), compressed_size);
}
- temp.push_back((checksum >> 24) & 0xFF);
- temp.push_back((checksum >> 16) & 0xFF);
- temp.push_back((checksum >> 8) & 0xFF);
- temp.push_back(checksum & 0xFF);
+ temp.push_back((char) ((checksum >> 24) & 0xFF));
Review Comment:
Again, please use `static_cast`.
##########
lang/c++/impl/DataFile.cc:
##########
@@ -522,18 +522,18 @@ void DataFileReaderBase::sync(int64_t position) {
DataFileSync sync_buffer;
const uint8_t *p = nullptr;
size_t n = 0;
- size_t i = 0;
+ int i = 0;
Review Comment:
It makes sense to keep `i` a `size_t` because it is used for indexing and
compared against the other `size_t` `SyncSize`.
##########
lang/c++/test/DataFileTests.cc:
##########
@@ -473,14 +474,14 @@ class DataFileTest {
avro::DataFileReader<ComplexInteger> df(filename, writerSchema);
std::ifstream just_for_length(
filename, std::ifstream::ate | std::ifstream::binary);
- int length = just_for_length.tellg();
+ std::streamoff length = just_for_length.tellg();
Review Comment:
Please use `auto`.
##########
lang/c++/impl/json/JsonIO.cc:
##########
@@ -363,24 +363,24 @@ string JsonParser::decodeString(const string &s, bool
binary) {
"Invalid byte for binary:
%1%%2%")
% ch % string(e, 4));
} else {
- result.push_back(n);
+ result.push_back((char) n);
continue;
}
}
if (n < 0x80) {
- result.push_back(n);
+ result.push_back((char) n);
Review Comment:
`static_cast` throughout the file.
--
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]
