[AVRO-4019] [C++] Turn on even more compiler warnings (#2966)

* [C++] Turn on -Wuseless-cast and -Wconversion

* Print versions of compiler/tools used in CI

* Fix another compiler warning

Also attempt to make build.sh continue

* Fix the last of the compiler warnings

* Update CodecTests.cc

* Update DataFileTests.cc

* Fix conversion warning on ARM64

* Hopefully make GCC 9.4 happy

* [C++] Address review comments

.github/workflows/test-lang-c++.yml

@@ -41,6 +41,12 @@ jobs:
       - name: Install Dependencies
         run: sudo apt update && sudo apt-get install -qqy cppcheck libboost-all-dev libsnappy-dev libfmt-dev cmake
 
+      - name: Print Versions
+        run: |
+          gcc --version
+          cmake --version
+          cppcheck --version
+
       - name: Clean
         run: ./build.sh clean
 

lang/c++/CMakeLists.txt

@@ -68,7 +68,7 @@ if (WIN32 AND NOT CYGWIN AND NOT MSYS)
 endif()
 
 if (CMAKE_COMPILER_IS_GNUCXX)
-    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -pedantic -Werror")
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Wduplicated-cond -Wduplicated-branches -Wlogical-op -Wuseless-cast -Wconversion -pedantic -Werror")
 if (AVRO_ADD_PROTECTOR_FLAGS)
     set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -fstack-protector-all -D_GLIBCXX_DEBUG")
     # Unset _GLIBCXX_DEBUG for avrogencpp.cc because using Boost Program Options

lang/c++/build.sh

@@ -83,7 +83,7 @@ case "$target" in
     ;;
 
   test)
-    (cmake -S. -Bbuild -D CMAKE_BUILD_TYPE=Debug -D AVRO_ADD_PROTECTOR_FLAGS=1 && cmake --build build \
+    (cmake -S. -Bbuild -D CMAKE_BUILD_TYPE=Debug -D AVRO_ADD_PROTECTOR_FLAGS=1 && cmake --build build -- -k \
       && ./build/buffertest \
       && ./build/unittest \
       && ./build/AvrogencppTestReservedWords \

lang/c++/impl/Compiler.cc

@@ -345,9 +345,10 @@ static LogicalType makeLogicalType(const Entity &e, const Object &m) {
     if (typeField == "decimal") {
         LogicalType decimalType(LogicalType::DECIMAL);
         try {
-            decimalType.setPrecision(getLongField(e, m, "precision"));
+            // Precision probably won't go over 38 and scale beyond -77/+77
+            decimalType.setPrecision(static_cast<int32_t>(getLongField(e, m, "precision")));
             if (containsField(m, "scale")) {
-                decimalType.setScale(getLongField(e, m, "scale"));
+                decimalType.setScale(static_cast<int32_t>(getLongField(e, m, "scale")));
             }
         } catch (Exception &ex) {
             // If any part of the logical type is malformed, per the standard we
@@ -395,12 +396,12 @@ 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"));
+    int64_t v = getLongField(e, m, "size");
     if (v <= 0) {
         throw Exception("Size for fixed is not positive: {}", e.toString());
     }
     NodePtr node =
-        NodePtr(new NodeFixed(asSingleAttribute(name), asSingleAttribute(v)));
+        NodePtr(new NodeFixed(asSingleAttribute(name), asSingleAttribute(static_cast<size_t>(v))));
     if (containsField(m, "doc")) {
         node->setDoc(getDocField(e, m));
     }

lang/c++/impl/DataFile.cc

@@ -195,10 +195,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(static_cast<char>((checksum >> 24) & 0xFF));
+        temp.push_back(static_cast<char>((checksum >> 16) & 0xFF));
+        temp.push_back(static_cast<char>((checksum >> 8) & 0xFF));
+        temp.push_back(static_cast<char>(checksum & 0xFF));
         std::unique_ptr<InputStream> in = memoryInputStream(
             reinterpret_cast<const uint8_t *>(temp.data()), temp.size());
         int64_t byteCount = temp.size();
@@ -455,7 +455,7 @@ static ValidSchema makeSchema(const vector<uint8_t> &v) {
     istringstream iss(toString(v));
     ValidSchema vs;
     compileJsonSchema(iss, vs);
-    return ValidSchema(vs);
+    return vs;
 }
 
 void DataFileReaderBase::readHeader() {
@@ -527,8 +527,7 @@ void DataFileReaderBase::sync(int64_t position) {
             eof_ = true;
             return;
         }
-        int len =
-            std::min(static_cast<size_t>(SyncSize - i), n);
+        size_t len = std::min(SyncSize - i, n);
         memcpy(&sync_buffer[i], p, len);
         p += len;
         n -= len;

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);
+        auto n = ::read(fd_, b, toRead);
         if (n > 0) {
             actual = n;
             return true;

lang/c++/impl/LogicalType.cc

@@ -28,7 +28,7 @@ LogicalType::Type LogicalType::type() const {
     return type_;
 }
 
-void LogicalType::setPrecision(int precision) {
+void LogicalType::setPrecision(int32_t precision) {
     if (type_ != DECIMAL) {
         throw Exception("Only logical type DECIMAL can have precision");
     }
@@ -38,7 +38,7 @@ void LogicalType::setPrecision(int precision) {
     precision_ = precision;
 }
 
-void LogicalType::setScale(int scale) {
+void LogicalType::setScale(int32_t scale) {
     if (type_ != DECIMAL) {
         throw Exception("Only logical type DECIMAL can have scale");
     }

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));
+                auto maxPrecision = static_cast<int32_t>(floor(log10(2.0) * (8.0 * static_cast<double>(fixedSize()) - 1)));
                 if (logicalType.precision() > maxPrecision) {
                     throw Exception(
                         "DECIMAL precision {} is too large for the "

lang/c++/impl/NodeImpl.cc

@@ -71,7 +71,7 @@ string escape(const string &unescaped) {
 // Wrap an indentation in a struct for ostream operator<<
 struct indent {
     explicit indent(size_t depth) : d(depth) {}
-    int d;
+    size_t d;
 };
 
 /// ostream operator for indent
@@ -83,7 +83,7 @@ std::ostream &operator<<(std::ostream &os, indent x) {
     return os;
 }
 
-void printCustomAttributes(const CustomAttributes &customAttributes, int depth,
+void printCustomAttributes(const CustomAttributes &customAttributes, size_t depth,
                            std::ostream &os) {
     std::map<std::string, std::string>::const_iterator iter =
         customAttributes.attributes().begin();
@@ -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();
+    auto names = leafNameAttributes_.size();
     ++depth;
-    for (int i = 0; i < names; ++i) {
+    for (size_t i = 0; i < names; ++i) {
         if (i > 0) {
             os << ",\n";
         }
@@ -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();
+    auto fields = leafAttributes_.size();
     ++depth;
-    for (int i = 0; i < fields; ++i) {
+    for (size_t i = 0; i < fields; ++i) {
         if (i > 0) {
             os << ",\n";
         }

lang/c++/impl/Resolver.cc

@@ -307,7 +307,7 @@ class EnumParser : public Resolver {
 
     void parse(Reader &reader, uint8_t *address) const final {
         auto val = static_cast<size_t>(reader.readEnum());
-        assert(static_cast<size_t>(val) < mapping_.size());
+        assert(val < mapping_.size());
 
         if (mapping_[val] < readerSize_) {
             auto *location = reinterpret_cast<EnumRepresentation *>(address + offset_);
@@ -349,7 +349,7 @@ class UnionParser : public Resolver {
 
         *readerChoice = choiceMapping_[writerChoice];
         auto *setter = reinterpret_cast<GenericUnionSetter *>(address + setFuncOffset_);
-        auto *value = reinterpret_cast<uint8_t *>(address + offset_);
+        uint8_t *value = address + offset_;
         uint8_t *location = (*setter)(value, *readerChoice);
 
         resolvers_[writerChoice]->parse(reader, location);
@@ -397,7 +397,7 @@ class NonUnionToUnionParser : public Resolver {
         auto *choice = reinterpret_cast<int64_t *>(address + choiceOffset_);
         *choice = choice_;
         auto *setter = reinterpret_cast<GenericUnionSetter *>(address + setFuncOffset_);
-        auto *value = reinterpret_cast<uint8_t *>(address + offset_);
+        uint8_t *value = address + offset_;
         uint8_t *location = (*setter)(value, choice_);
 
         resolver_->parse(reader, location);
@@ -424,7 +424,7 @@ class FixedSkipper : public Resolver {
     }
 
 protected:
-    int size_;
+    size_t size_;
 };
 
 class FixedParser : public Resolver {
@@ -436,12 +436,12 @@ class FixedParser : public Resolver {
 
     void parse(Reader &reader, uint8_t *address) const final {
         DEBUG_OUT("Reading fixed");
-        auto *location = reinterpret_cast<uint8_t *>(address + offset_);
+        uint8_t *location = address + offset_;
         reader.readFixed(location, size_);
     }
 
 protected:
-    int size_;
+    size_t size_;
     size_t offset_;
 };
 

lang/c++/impl/Validator.cc

@@ -62,7 +62,7 @@ bool Validator::countingSetup() {
             compoundStack_.pop_back();
             proceed = false;
         } else {
-            counters_.push_back(static_cast<size_t>(count_));
+            counters_.push_back(count_);
         }
     }
 
@@ -78,7 +78,7 @@ void Validator::countingAdvance() {
             setupOperation(node->leafAt(index));
         } else {
             compoundStack_.back().pos = 0;
-            int count = --counters_.back();
+            size_t count = --counters_.back();
             if (count == 0) {
                 counters_.pop_back();
                 compoundStarted_ = true;
@@ -100,7 +100,7 @@ void Validator::unionAdvance() {
         waitingForCount_ = false;
         NodePtr node = compoundStack_.back().node;
 
-        if (count_ < static_cast<int64_t>(node->leaves())) {
+        if (count_ < node->leaves()) {
             compoundStack_.pop_back();
             setupOperation(node->leafAt(static_cast<int>(count_)));
         } else {
@@ -116,7 +116,7 @@ void Validator::fixedAdvance() {
     compoundStack_.pop_back();
 }
 
-int Validator::nextSizeExpected() const {
+size_t Validator::nextSizeExpected() const {
     return compoundStack_.back().node->fixedSize();
 }
 
@@ -168,11 +168,9 @@ void Validator::advance() {
     }
 }
 
-void Validator::setCount(int64_t count) {
+void Validator::setCount(size_t count) {
     if (!waitingForCount_) {
         throw Exception("Not expecting count");
-    } else if (count_ < 0) {
-        throw Exception("Count cannot be negative");
     }
     count_ = count;
 

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++] = static_cast<uint8_t>(v | 0x80);
         v = val & mask;
     }
 
-    output[bytesOut++] = v;
+    output[bytesOut++] = static_cast<uint8_t>(v);
     return bytesOut;
 }
 size_t
@@ -46,11 +46,11 @@ encodeInt32(int32_t input, std::array<uint8_t, 5> &output) noexcept {
     auto v = val & mask;
     size_t bytesOut = 0;
     while (val >>= 7) {
-        output[bytesOut++] = (v | 0x80);
+        output[bytesOut++] = static_cast<uint8_t>(v | 0x80);
         v = val & mask;
     }
 
-    output[bytesOut++] = v;
+    output[bytesOut++] = static_cast<uint8_t>(v);
     return bytesOut;
 }
 

lang/c++/impl/json/JsonIO.cc

@@ -378,7 +378,7 @@ string JsonParser::decodeString(const string &s, bool binary) {
                         if (n > 0xff) {
                             throw Exception("Invalid byte for binary: {}{}", ch, string(startSeq, ++it));
                         } else {
-                            result.push_back(n);
+                            result.push_back(static_cast<char>(n));
                             continue;
                         }
                     }
@@ -400,19 +400,19 @@ string JsonParser::decodeString(const string &s, bool binary) {
                         throw Exception("Invalid unicode sequence: {}", string(startSeq, it));
                     }
                     if (n < 0x80) {
-                        result.push_back(n);
+                        result.push_back(static_cast<char>(n));
                     } else if (n < 0x800) {
-                        result.push_back((n >> 6) | 0xc0);
-                        result.push_back((n & 0x3f) | 0x80);
+                        result.push_back(static_cast<char>((n >> 6) | 0xc0));
+                        result.push_back(static_cast<char>((n & 0x3f) | 0x80));
                     } else if (n < 0x10000) {
-                        result.push_back((n >> 12) | 0xe0);
-                        result.push_back(((n >> 6) & 0x3f) | 0x80);
-                        result.push_back((n & 0x3f) | 0x80);
+                        result.push_back(static_cast<char>((n >> 12) | 0xe0));
+                        result.push_back(static_cast<char>(((n >> 6) & 0x3f) | 0x80));
+                        result.push_back(static_cast<char>((n & 0x3f) | 0x80));
                     } else if (n < 0x110000) {
-                        result.push_back((n >> 18) | 0xf0);
-                        result.push_back(((n >> 12) & 0x3f) | 0x80);
-                        result.push_back(((n >> 6) & 0x3f) | 0x80);
-                        result.push_back((n & 0x3f) | 0x80);
+                        result.push_back(static_cast<char>((n >> 18) | 0xf0));
+                        result.push_back(static_cast<char>(((n >> 12) & 0x3f) | 0x80));
+                        result.push_back(static_cast<char>(((n >> 6) & 0x3f) | 0x80));
+                        result.push_back(static_cast<char>((n & 0x3f) | 0x80));
                     } else {
                         throw Exception("Invalid unicode value: {}{}", n, string(startSeq, ++it));
                     }

lang/c++/impl/json/JsonIO.hh

@@ -34,7 +34,7 @@ namespace avro {
 namespace json {
 
 inline char toHex(unsigned int n) {
-    return (n < 10) ? (n + '0') : (n + 'a' - 10);
+    return static_cast<char>((n < 10) ? (n + '0') : (n + 'a' - 10));
 }
 
 class AVRO_DECL JsonParser : boost::noncopyable {
@@ -266,8 +266,8 @@ class AVRO_DECL JsonGenerator {
     void escapeUnicode16(uint32_t c) {
         out_.write('\\');
         out_.write('u');
-        writeHex((c >> 8) & 0xff);
-        writeHex(c & 0xff);
+        writeHex(static_cast<char>((c >> 8) & 0xff));
+        writeHex(static_cast<char>(c & 0xff));
     }
     void escapeUnicode(uint32_t c) {
         if (c < 0x10000) {