You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by GitBox <gi...@apache.org> on 2023/01/09 03:41:49 UTC
[GitHub] [datasketches-cpp] jbapple opened a new pull request, #324: Remove write-only variables
jbapple opened a new pull request, #324:
URL: https://github.com/apache/datasketches-cpp/pull/324
These variables were never read, which causes warning when compiling with clang 16
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] jmalkin commented on pull request #324: Remove write-only variables
Posted by GitBox <gi...@apache.org>.
jmalkin commented on PR #324:
URL: https://github.com/apache/datasketches-cpp/pull/324#issuecomment-1376028816
waiting for the CI job, then will merge
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] jbapple commented on a diff in pull request #324: Remove write-only variables
Posted by GitBox <gi...@apache.org>.
jbapple commented on code in PR #324:
URL: https://github.com/apache/datasketches-cpp/pull/324#discussion_r1064916022
##########
sampling/include/var_opt_sketch_impl.hpp:
##########
@@ -752,18 +752,16 @@ string<A> var_opt_sketch<T, A>::items_to_string(bool print_gap) const {
std::ostringstream os;
os << "### Sketch Items" << std::endl;
const uint32_t array_length = (n_ < k_ ? n_ : k_ + 1);
- for (uint32_t i = 0, display_idx = 0; i < array_length; ++i) {
Review Comment:
Found it when compiling the library and running the included tests.
Changed to now use `display_idx` in the output.
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] jmalkin commented on a diff in pull request #324: Remove write-only variables
Posted by GitBox <gi...@apache.org>.
jmalkin commented on code in PR #324:
URL: https://github.com/apache/datasketches-cpp/pull/324#discussion_r1064379687
##########
sampling/include/var_opt_sketch_impl.hpp:
##########
@@ -752,18 +752,16 @@ string<A> var_opt_sketch<T, A>::items_to_string(bool print_gap) const {
std::ostringstream os;
os << "### Sketch Items" << std::endl;
const uint32_t array_length = (n_ < k_ ? n_ : k_ + 1);
- for (uint32_t i = 0, display_idx = 0; i < array_length; ++i) {
Review Comment:
i think `os << i` should actually have been `os << display_idx` in the loop body
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] coveralls commented on pull request #324: Remove write-only variables
Posted by GitBox <gi...@apache.org>.
coveralls commented on PR #324:
URL: https://github.com/apache/datasketches-cpp/pull/324#issuecomment-1375071185
## Pull Request Test Coverage Report for [Build 3870562108](https://coveralls.io/builds/55745213)
* **0** of **0** changed or added relevant lines in **0** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage remained the same at **93.868%**
---
| Totals | [](https://coveralls.io/builds/55745213) |
| :-- | --: |
| Change from base [Build 3849847839](https://coveralls.io/builds/55684869): | 0.0% |
| Covered Lines: | 2327 |
| Relevant Lines: | 2479 |
---
##### 💛 - [Coveralls](https://coveralls.io)
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] jmalkin commented on pull request #324: Remove write-only variables
Posted by GitBox <gi...@apache.org>.
jmalkin commented on PR #324:
URL: https://github.com/apache/datasketches-cpp/pull/324#issuecomment-1375263982
Thanks for reporting this!
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] jmalkin commented on a diff in pull request #324: Remove write-only variables
Posted by GitBox <gi...@apache.org>.
jmalkin commented on code in PR #324:
URL: https://github.com/apache/datasketches-cpp/pull/324#discussion_r1064930859
##########
sampling/include/var_opt_sketch_impl.hpp:
##########
@@ -752,18 +752,16 @@ string<A> var_opt_sketch<T, A>::items_to_string(bool print_gap) const {
std::ostringstream os;
os << "### Sketch Items" << std::endl;
const uint32_t array_length = (n_ < k_ ? n_ : k_ + 1);
- for (uint32_t i = 0, display_idx = 0; i < array_length; ++i) {
Review Comment:
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] jbapple commented on a diff in pull request #324: Remove write-only variables
Posted by GitBox <gi...@apache.org>.
jbapple commented on code in PR #324:
URL: https://github.com/apache/datasketches-cpp/pull/324#discussion_r1064779030
##########
sampling/include/var_opt_sketch_impl.hpp:
##########
@@ -752,18 +752,16 @@ string<A> var_opt_sketch<T, A>::items_to_string(bool print_gap) const {
std::ostringstream os;
os << "### Sketch Items" << std::endl;
const uint32_t array_length = (n_ < k_ ? n_ : k_ + 1);
- for (uint32_t i = 0, display_idx = 0; i < array_length; ++i) {
Review Comment:
Both places or just one?
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] jmalkin commented on a diff in pull request #324: Remove write-only variables
Posted by GitBox <gi...@apache.org>.
jmalkin commented on code in PR #324:
URL: https://github.com/apache/datasketches-cpp/pull/324#discussion_r1064868895
##########
sampling/include/var_opt_sketch_impl.hpp:
##########
@@ -752,18 +752,16 @@ string<A> var_opt_sketch<T, A>::items_to_string(bool print_gap) const {
std::ostringstream os;
os << "### Sketch Items" << std::endl;
const uint32_t array_length = (n_ < k_ ? n_ : k_ + 1);
- for (uint32_t i = 0, display_idx = 0; i < array_length; ++i) {
Review Comment:
Both
Not sure if you found this using this sketch or when compiling the library. The way it works, which isn't a necessary thing but helps simplify the algo, is that there's an array of size k+1, with a gap of size 1 between two portions of the sampling array that get slightly different treatment (heavy items vs items treated more like uniform reservoir sample). `to_string()` methods are largely just for debugging so providing an option to more easily track where that gap lives is useful -- but it changes indexing for items after the gap.
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] jmalkin merged pull request #324: Remove write-only variables
Posted by GitBox <gi...@apache.org>.
jmalkin merged PR #324:
URL: https://github.com/apache/datasketches-cpp/pull/324
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org