You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2022/08/23 12:47:41 UTC
[GitHub] [avro] martin-g opened a new pull request, #1836: AVRO-3616: C++ Fix compilation warnings
martin-g opened a new pull request, #1836:
URL: https://github.com/apache/avro/pull/1836
### Jira
- [X] https://issues.apache.org/jira/browse/AVRO-3616
### Tests
- [X] My PR updates existing unit tests
### Commits
- [X] My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](https://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation
- [X] No need
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a diff in pull request #1836: AVRO-3616: C++ Fix compilation warnings
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1836:
URL: https://github.com/apache/avro/pull/1836#discussion_r953446652
##########
lang/c++/impl/ValidSchema.cc:
##########
@@ -70,7 +70,7 @@ static bool validate(const NodePtr &node, SymbolMap &symbolMap) {
node->lock();
auto leaves = node->leaves();
- for (auto i = 0; i < leaves; ++i) {
+ for (long unsigned int i = 0; i < leaves; ++i) {
Review Comment:
Improved! 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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] KalleOlaviNiemitalo commented on a diff in pull request #1836: AVRO-3616: C++ Fix compilation warnings
Posted by GitBox <gi...@apache.org>.
KalleOlaviNiemitalo commented on code in PR #1836:
URL: https://github.com/apache/avro/pull/1836#discussion_r952780342
##########
lang/c++/impl/ValidSchema.cc:
##########
@@ -70,7 +70,7 @@ static bool validate(const NodePtr &node, SymbolMap &symbolMap) {
node->lock();
auto leaves = node->leaves();
- for (auto i = 0; i < leaves; ++i) {
+ for (long unsigned int i = 0; i < leaves; ++i) {
Review Comment:
On 64-bit Microsoft Windows, `size_t` is 64-bit, but `long unsigned int` is 32-bit and would not be pedantically correct. In practice though, it's rather unlikely that a node would have billions of leaves.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on pull request #1836: AVRO-3616: C++ Fix compilation warnings
Posted by GitBox <gi...@apache.org>.
martin-g commented on PR #1836:
URL: https://github.com/apache/avro/pull/1836#issuecomment-1225315738
Thanks, @KalleOlaviNiemitalo !
I will leave this for a separate ticket/PR.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] KalleOlaviNiemitalo commented on pull request #1836: AVRO-3616: C++ Fix compilation warnings
Posted by GitBox <gi...@apache.org>.
KalleOlaviNiemitalo commented on PR #1836:
URL: https://github.com/apache/avro/pull/1836#issuecomment-1225322361
I filed those as [AVRO-3617](https://issues.apache.org/jira/browse/AVRO-3617).
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] KalleOlaviNiemitalo commented on pull request #1836: AVRO-3616: C++ Fix compilation warnings
Posted by GitBox <gi...@apache.org>.
KalleOlaviNiemitalo commented on PR #1836:
URL: https://github.com/apache/avro/pull/1836#issuecomment-1225313705
I worried that commit e632e652f3b5805609647827b9f4611e79185c6e changing the types of the local variables to size_t in might cause problems if any subsequent code tries to use negative values in those variables. I looked at the remaining parts of the modified methods and did not find any such problems, but found the `int count = --counters_.back()` assignment in Validator::countingAdvance. That seemed suspicious to me, so I searched for other uses of `counters_` and `count_` in the Validator files. I didn't even use an IDE, just a Web browser.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] KalleOlaviNiemitalo commented on a diff in pull request #1836: AVRO-3616: C++ Fix compilation warnings
Posted by GitBox <gi...@apache.org>.
KalleOlaviNiemitalo commented on code in PR #1836:
URL: https://github.com/apache/avro/pull/1836#discussion_r952777202
##########
lang/c++/impl/ValidSchema.cc:
##########
@@ -70,7 +70,7 @@ static bool validate(const NodePtr &node, SymbolMap &symbolMap) {
node->lock();
auto leaves = node->leaves();
- for (auto i = 0; i < leaves; ++i) {
+ for (long unsigned int i = 0; i < leaves; ++i) {
Review Comment:
Better use size_t, because Node::leaves returns size_t, and Node::leafAt takes a size_t parameter. Or perhaps decltype(leaves).
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] KalleOlaviNiemitalo commented on pull request #1836: AVRO-3616: C++ Fix compilation warnings
Posted by GitBox <gi...@apache.org>.
KalleOlaviNiemitalo commented on PR #1836:
URL: https://github.com/apache/avro/pull/1836#issuecomment-1225303643
In Validator, there seems to be some inconsistency with `std::vector<size_t> counters_` and `int64_t count_`, too:
- Validator::countingSetup converts int64_t to size_t: `counters_.push_back(static_cast<size_t>(count_));`
- Validator::countingAdvance converts size_t to int: `int count = --counters_.back();`
- Validator::unionAdvance converts size_t to int64_t: `if (count_ < static_cast<int64_t>(node->leaves()))`
- Validator::unionAdvance converts int64_t to int and that to size_t: `setupOperation(node->leafAt(static_cast<int>(count_)));`
Those could be fixed in a separate PR, though.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on pull request #1836: AVRO-3616: C++ Fix compilation warnings
Posted by GitBox <gi...@apache.org>.
martin-g commented on PR #1836:
URL: https://github.com/apache/avro/pull/1836#issuecomment-1225309037
> In Validator, there seems to be some inconsistency with
How did you find those ?
By looking in the code or by using some tool ?
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g merged pull request #1836: AVRO-3616: C++ Fix compilation warnings
Posted by GitBox <gi...@apache.org>.
martin-g merged PR #1836:
URL: https://github.com/apache/avro/pull/1836
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org