Skip to content

Commit

Permalink
[AVRO-4019] [C++] Turn on even more compiler warnings (#2966)
Browse files Browse the repository at this point in the history
* [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
  • Loading branch information
Gerrit0 authored Jul 16, 2024
1 parent 8dcd9c7 commit c460d64
Show file tree
Hide file tree
Showing 29 changed files with 136 additions and 126 deletions.
6 changes: 6 additions & 0 deletions .github/workflows/test-lang-c++.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion lang/c++/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lang/c++/build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down
9 changes: 5 additions & 4 deletions lang/c++/impl/Compiler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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));
}
Expand Down
13 changes: 6 additions & 7 deletions lang/c++/impl/DataFile.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion lang/c++/impl/FileStream.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions lang/c++/impl/LogicalType.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand All @@ -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");
}
Expand Down
2 changes: 1 addition & 1 deletion lang/c++/impl/Node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 "
Expand Down
12 changes: 6 additions & 6 deletions lang/c++/impl/NodeImpl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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();
Expand Down Expand Up @@ -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";
}
Expand Down Expand Up @@ -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";
}
Expand Down
12 changes: 6 additions & 6 deletions lang/c++/impl/Resolver.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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_);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -424,7 +424,7 @@ class FixedSkipper : public Resolver {
}

protected:
int size_;
size_t size_;
};

class FixedParser : public Resolver {
Expand All @@ -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_;
};

Expand Down
12 changes: 5 additions & 7 deletions lang/c++/impl/Validator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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_);
}
}

Expand All @@ -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;
Expand All @@ -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 {
Expand All @@ -116,7 +116,7 @@ void Validator::fixedAdvance() {
compoundStack_.pop_back();
}

int Validator::nextSizeExpected() const {
size_t Validator::nextSizeExpected() const {
return compoundStack_.back().node->fixedSize();
}

Expand Down Expand Up @@ -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;

Expand Down
8 changes: 4 additions & 4 deletions lang/c++/impl/Zigzag.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
}

Expand Down
22 changes: 11 additions & 11 deletions lang/c++/impl/json/JsonIO.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand All @@ -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));
}
Expand Down
6 changes: 3 additions & 3 deletions lang/c++/impl/json/JsonIO.hh
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand Down
Loading

0 comments on commit c460d64

Please sign in to comment.