Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AVRO-3617: reduce cast warning #1852

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

AVRO-3617: reduce cast warning #1852

wants to merge 5 commits into from

Conversation

clesaec
Copy link
Contributor

@clesaec clesaec commented Sep 2, 2022

Try for AVRO-3617

@github-actions github-actions bot added the C++ Pull Requests for C++ binding label Sep 2, 2022
thiru-mg
thiru-mg reviewed Oct 16, 2022
View reviewed changes
Copy link
Contributor

@thiru-mg thiru-mg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks basically good. But there are issues, especially C-style cast.

lang/c++/CMakeLists.txt Outdated
@@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make indentation here consistent with the rest of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

lang/c++/api/Validator.hh Outdated
@@ -142,7 +142,7 @@ private:
};

std::vector<CompoundType> compoundStack_;
std::vector<size_t> counters_;
std::vector<int64_t> counters_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the counters cannot be negative, should this be unit64_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

lang/c++/api/buffer/BufferStreambuf.hh Outdated
memcpy(c, gptr(), toCopy);
c += toCopy;
bytesCopied += toCopy;
gbump(toCopy);
gbump((int) toCopy);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

lang/c++/impl/Compiler.cc Outdated
@@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In modern C++ we try to avoid explicitly specifying type in situations like this: please use auto.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

lang/c++/impl/DataFile.cc Outdated
temp.push_back((checksum >> 16) & 0xFF);
temp.push_back((checksum >> 8) & 0xFF);
temp.push_back(checksum & 0xFF);
temp.push_back((char) ((checksum >> 24) & 0xFF));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, please use static_cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

lang/c++/impl/DataFile.cc Outdated
@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to keep i a size_t because it is used for indexing and compared against the other size_t SyncSize.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

lang/c++/impl/Zigzag.cc Outdated
@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, please do static_cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

lang/c++/impl/json/JsonIO.cc Outdated
continue;
}
}
if (n < 0x80) {
result.push_back(n);
result.push_back((char) n);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static_cast throughout the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

lang/c++/test/DataFileTests.cc Outdated
@@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use auto.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@clesaec
Copy link
Contributor Author

clesaec commented Oct 17, 2022

All changes done.

@clesaec clesaec requested a review from thiru-mg January 4, 2023 07:38
@clesaec
Copy link
Contributor Author

clesaec commented Jun 12, 2023

friendly remlinder @thiru-mg,
Do you finally approve this PR ?
Is there any commiter who can see it ?

@Gerrit0 Gerrit0 mentioned this pull request Jun 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thiru-mg thiru-mg Awaiting requested review from thiru-mg

Assignees
No one assigned
Labels
C++ Pull Requests for C++ binding
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@clesaec @thiru-mg