Negative ranges #762#787
Merged
dmah42 merged 8 commits intogoogle:masterfrom Mar 26, 2019
danielharvey458:negative_ranges_#762
Merged
Negative ranges #762#787dmah42 merged 8 commits intogoogle:masterfrom danielharvey458:negative_ranges_#762
dmah42 merged 8 commits intogoogle:masterfrom
danielharvey458:negative_ranges_#762
Conversation
Due to breaking the loop too early, AddRange would miss a final multplier of 'mult' that was within the numeric range of T.
danielharvey458
commented
Mar 24, 2019
dmah42
requested changes
Mar 25, 2019
|
|
||
| namespace { | ||
|
|
||
| using namespace benchmark::internal; |
Member
There was a problem hiding this comment.
these tests should likely be in namespace benchmark::internal which you can add above.
namespace benchmark {
namespace internal {
namespace {
...
} // namespace
} // namespace internal
} // namespace benchmark| // complete. | ||
| virtual ~MultipleRangesFixture() { | ||
| // FIXME Should the 'if' below be checking for equality | ||
| // of the values in the containers, rather than their size? |
Member
There was a problem hiding this comment.
it should probably do both. The assert is no longer necessary as we're using for range loops (we used to loop using the index and it would crash) and instead we should be looping over both and comparing, then asserting/aborting with some sort of error.
Put unit tests in benchmark::internal namespace Fix error reporting in multiple_ranges_test.cc
dmah42
approved these changes
Mar 25, 2019
Member
|
Thank you! |
JBakamovic
pushed a commit
to JBakamovic/benchmark
that referenced
this pull request
Sep 11, 2020
* Add FIXME in multiple_ranges_test.cc * Improve handling of large bounds in AddRange. Due to breaking the loop too early, AddRange would miss a final multplier of 'mult' that was within the numeric range of T. * Enable negative values for Range argument Fixes google#762. * Try to fix build of benchmark_gtest * Try some more to fix build * Attempt to fix format macros * Attempt to resolve format errors for mingw32 * Review feedback Put unit tests in benchmark::internal namespace Fix error reporting in multiple_ranges_test.cc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add FIXME in multiple_ranges_test.cc 36c7640 -- this looks like a bug to me?
Improve handling of large bounds in AddRange 02a67f0 -- minor tweak to squeeze in an extra power of 'mult' in AddRanges
Enable negative values for Range argument 2ddf958 -- main proposed commit