remove narrowing conversion in king_ordering.hpp#499
Conversation
Becheler
added
warning
priority: high
|
Compiler-warning counts vs
|
jeremy-murphy
left a comment
jeremy-murphy
left a comment
There was a problem hiding this comment.
Definite improvement to have proper unit tests, but a few comments and questions about code choices. :)
| child_location = parent_location; | ||
| heap_child_location = heap_parent_location; | ||
| heap_parent_location = (int)(heap_child_location / 2); | ||
| heap_parent_location = (std::ptrdiff_t)(heap_child_location / 2); |
There was a problem hiding this comment.
This cast is redundant.
| heap_parent_location = (std::ptrdiff_t)(heap_child_location / 2); | |
| heap_parent_location = heap_child_location / 2; |
| << std::endl; | ||
| size_type bw = bandwidth( | ||
| G, make_iterator_property_map(&perm[0], index_map, perm[0])); | ||
| std::cout << " bandwidth: " << bw << std::endl; |
There was a problem hiding this comment.
I would just remove all the console output as it's just noise 99% of the time. If we need console output only occasionally, then it should use a logger with configurable output levels so that a developer can easily turn it on during development.
So wait... was this test not actually testing anything, just printing console output? 😅
| auto assert_valid_permutation | ||
| = [&index_map, n](const std::vector< Vertex >& order) { | ||
| BOOST_TEST(order.size() == n); | ||
| std::vector< bool > seen(n, false); |
There was a problem hiding this comment.
This particular use is OK, but in general prefer boost::dynamic_bitset.
| std::ptrdiff_t heap_parent_location | ||
| = (std::ptrdiff_t)(heap_child_location / 2); |
There was a problem hiding this comment.
This cast is redundant.
| std::ptrdiff_t heap_parent_location | |
| = (std::ptrdiff_t)(heap_child_location / 2); | |
| std::ptrdiff_t heap_parent_location = heap_child_location / 2; |
| public: | ||
| bfs_king_visitor(OutputIterator* iter, Buffer* b, Compare compare, | ||
| PseudoDegreeMap deg, std::vector< int > loc, VecMap color, | ||
| PseudoDegreeMap deg, std::vector< std::ptrdiff_t > loc, VecMap color, |
There was a problem hiding this comment.
Why do you think this is std::ptrdiff_t?
I've looked at the code, and I'm having trouble figuring out what it should be, but I'm inclined to think it should be std::size_t, unless there is a requirement for it to be signed?
2 participants
Refs #496
Before submitting
developbranch.Type of change
Does this PR introduce a breaking change?
What this PR does
Two commits:
king_ordering.cppunit test (it did not assert anything) to prevent regressionsking_ordering.hppheap frominttostd::ptrdiff_tso thesize_tqueue positions no longer narrowMotivation
Refs #496
Testing
Same CI, more hardened tests, less warnings:
Checklist
b2in thetest/directory).