Skip to content

Commit

Permalink
🧹 Fix GCC complaints (#2400)
Browse files Browse the repository at this point in the history
# Tree

Fixes --- likely a spurious --- warning in `tree.cpp`:
```
[2/6] Building CXX object arbor/CMakeFiles/arbor.dir/tree.cpp.o
In file included from /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/algorithm:60,
                 from /Users/hater/src/arbor/arbor/tree.cpp:1:
In function 'typename __gnu_cxx::__enable_if<std::__is_scalar<_Tp>::__value, void>::__type std::__fill_a1(_ForwardIterator, _ForwardIterator, const _Tp&) [with _ForwardIterator = unsigned int*; _Tp = unsigned int]',
    inlined from 'void std::__fill_a(_FIte, _FIte, const _Tp&) [with _FIte = unsigned int*; _Tp = unsigned int]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algobase.h:998:21,
    inlined from '_OutputIterator std::__fill_n_a(_OutputIterator, _Size, const _Tp&, random_access_iterator_tag) [with _OutputIterator = unsigned int*; _Size = long unsigned int; _Tp = unsigned int]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algobase.h:1151:20,
    inlined from '_OI std::fill_n(_OI, _Size, const _Tp&) [with _OI = unsigned int*; _Size = long unsigned int; _Tp = unsigned int]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algobase.h:1180:29,
    inlined from 'static _ForwardIterator std::__uninitialized_default_n_1<true>::__uninit_default_n(_ForwardIterator, _Size) [with _ForwardIterator = unsigned int*; _Size = long unsigned int]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_uninitialized.h:668:29,
    inlined from 'static _ForwardIterator std::__uninitialized_default_n_1<true>::__uninit_default_n(_ForwardIterator, _Size) [with _ForwardIterator = unsigned int*; _Size = long unsigned int]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_uninitialized.h:660:9,
    inlined from '_ForwardIterator std::__uninitialized_default_n(_ForwardIterator, _Size) [with _ForwardIterator = unsigned int*; _Size = long unsigned int]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_uninitialized.h:712:20,
    inlined from '_ForwardIterator std::__uninitialized_default_n_a(_ForwardIterator, _Size, allocator<_Tp>&) [with _ForwardIterator = unsigned int*; _Size = long unsigned int; _Tp = unsigned int]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_uninitialized.h:779:44,
    inlined from 'void std::vector<_Tp, _Alloc>::_M_default_initialize(size_type) [with _Tp = unsigned int; _Alloc = std::allocator<unsigned int>]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_vector.h:1720:36,
    inlined from 'std::vector<_Tp, _Alloc>::vector(size_type, const allocator_type&) [with _Tp = unsigned int; _Alloc = std::allocator<unsigned int>]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_vector.h:558:30,
    inlined from 'arb::tree::iarray arb::tree::select_new_root(int_type)' at /Users/hater/src/arbor/arbor/tree.cpp:219:34:
/opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algobase.h:952:18: warning: 'void* __builtin_memset(void*, int, long unsigned int)' offset 4 is out of the bounds [0, 0] [-Warray-bounds=]
  952 |         *__first = __tmp;
```
Also use `iota` to generate contiguous indices.

# Tests

## `test_simd.cpp`

GCC doesn't acknowledge that `simd v[i] = x` for all `i = 0 ... len(v)`
initializes `v`. Force initialization to zero.

## `test_network_generation.cpp`

Compare across signs

## `test_distributed_for_each.cpp`

Compare across signs; resolve by casting to correct type.   

## `test_range.cpp`

GCC really turns the dial up to 11 here, complaining about oob access in
```
char str[] = "howdy";
auto rg = range_n(str, strlen(str)); // NOTE: Putting 5 here is OK, so is sizeof(str) - 1
util::sort(rg);
```
Same happens with `std::sort(str, str + strlen(str))`, so I am pretty
sure this is not a real thing.
For reference:
```
/Users/hater/src/arbor/test/unit/test_range.cpp: In member function 'virtual void range_sort_Test::TestBody()':
/Users/hater/src/arbor/test/unit/test_range.cpp:446:14: note: at offset [8, 4611686018427387903] into object 'cstr' of size 6
  446 |         char cstr[] = "howdy";
      |              ^~~~
In function 'std::_Require<std::__not_<std::__is_tuple_like<_Tp> >, std::is_move_constructible<_Tp>, std::is_move_assignable<_Tp> > std::swap(_Tp&, _Tp&) [with _Tp = char]',
    inlined from 'void std::iter_swap(_ForwardIterator1, _ForwardIterator2) [with _ForwardIterator1 = char*; _ForwardIterator2 = char*]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algobase.h:185:11,
    inlined from 'void std::__move_median_to_first(_Iterator, _Iterator, _Iterator, _Iterator, _Compare) [with _Iterator = char*; _Compare = __gnu_cxx::__ops::_Iter_less_iter]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algo.h:91:20,
    inlined from '_RandomAccessIterator std::__unguarded_partition_pivot(_RandomAccessIterator, _RandomAccessIterator, _Compare) [with _RandomAccessIterator = char*; _Compare = __gnu_cxx::__ops::_Iter_less_iter]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algo.h:1855:34,
    inlined from 'void std::__introsort_loop(_RandomAccessIterator, _RandomAccessIterator, _Size, _Compare) [with _RandomAccessIterator = char*; _Size = long int; _Compare = __gnu_cxx::__ops::_Iter_less_iter]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algo.h:1889:38,
    inlined from 'void std::__sort(_RandomAccessIterator, _RandomAccessIterator, _Compare) [with _RandomAccessIterator = char*; _Compare = __gnu_cxx::__ops::_Iter_less_iter]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algo.h:1905:25,
    inlined from 'void std::__sort(_RandomAccessIterator, _RandomAccessIterator, _Compare) [with _RandomAccessIterator = char*; _Compare = __gnu_cxx::__ops::_Iter_less_iter]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algo.h:1900:5,
    inlined from 'void std::sort(_RAIter, _RAIter) [with _RAIter = char*]' at /opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/stl_algo.h:4771:18,
    inlined from 'std::enable_if_t<((bool)(! std::is_const<typename arb::util::impl_seqtrait::sequence_traits<Seq&&>::reference>::value))> arb::util::sort(Seq&&) [with Seq = range<char*, char*>]' at /Users/hater/src/arbor/arbor/util/rangeutil.hpp:154:14,
    inlined from 'virtual void range_sort_Test::TestBody()' at /Users/hater/src/arbor/test/unit/test_range.cpp:448:19:
/opt/homebrew/Cellar/gcc/14.2.0/include/c++/14/bits/move.h:223:11: warning: array subscript [8, 4611686018427387903] is outside array bounds of 'char [6]' [-Warray-bounds=]
  223 |       __b = _GLIBCXX_MOVE(__tmp);
      |           ^
/Users/hater/src/arbor/test/unit/test_range.cpp: In member function 'virtual void range_sort_Test::TestBody()':
/Users/hater/src/arbor/test/unit/test_range.cpp:446:14: note: at offset [8, 4611686018427387903] into object 'cstr' of size 6
  446 |         char cstr[] = "howdy";
```

I am still going to 'fix' this, since I am for warning free code and
don't want to annotate with a pragma.

# General

Remove spurious includes.

---------

Co-authored-by: boeschf <[email protected]>
  • Loading branch information
thorstenhater and boeschf authored Oct 24, 2024
1 parent b8b768d commit 9ba170a
Show file tree
Hide file tree
Showing 8 changed files with 55 additions and 75 deletions.
28 changes: 11 additions & 17 deletions arbor/tree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ tree::tree(std::vector<tree::int_type> parent_index) {
parents_[0] = no_parent;

// compute offsets into children_ array
arb::util::make_partition(child_index_, child_count(parents_));
util::make_partition(child_index_, child_count(parents_));

std::vector<int_type> pos(parents_.size(), 0);
for (auto i = 1u; i < parents_.size(); ++i) {
Expand Down Expand Up @@ -199,11 +199,9 @@ tree::iarray tree::select_new_root(int_type root) {
}

// maps new indices to old indices
iarray indices (num_nodes);
// fill array with indices
for (auto i: make_span(num_nodes)) {
indices[i] = i;
}
iarray indices(num_nodes);
std::iota(indices.begin(), indices.end(), 0);

// perform sort by depth index to get the permutation
std::sort(indices.begin(), indices.end(), [&](auto i, auto j){
if (reduced_depth[i] != reduced_depth[j]) {
Expand All @@ -214,16 +212,12 @@ tree::iarray tree::select_new_root(int_type root) {
}
return depth[i] < depth[j];
});
// maps old indices to new indices
iarray indices_inv (num_nodes);
// fill array with indices
for (auto i: make_span(num_nodes)) {
indices_inv[i] = i;
}
// perform sort
std::sort(indices_inv.begin(), indices_inv.end(), [&](auto i, auto j){
return indices[i] < indices[j];
});

// inverse permutation
iarray indices_inv(num_nodes, 0);
std::iota(indices_inv.begin(), indices_inv.end(), 0);
std::sort(indices_inv.begin(), indices_inv.end(),
[&](auto i, auto j){ return indices[i] < indices[j]; });

// translate the parent vetor to new indices
for (auto i: make_span(num_nodes)) {
Expand All @@ -241,7 +235,7 @@ tree::iarray tree::select_new_root(int_type root) {

// recompute the children array
memory::copy(new_parents, parents_);
arb::util::make_partition(child_index_, child_count(parents_));
util::make_partition(child_index_, child_count(parents_));

std::vector<int_type> pos(parents_.size(), 0);
for (auto i = 1u; i < parents_.size(); ++i) {
Expand Down
4 changes: 0 additions & 4 deletions arbor/tree.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,12 @@

#include <algorithm>
#include <cassert>
#include <fstream>
#include <numeric>
#include <vector>

#include <arbor/export.hpp>
#include <arbor/common_types.hpp>

#include "memory/memory.hpp"
#include "util/rangeutil.hpp"
#include "util/span.hpp"

namespace arb {

Expand Down
8 changes: 3 additions & 5 deletions arbor/util/range.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,9 @@ template <typename Seq>
auto canonical_view(Seq&& s) {
using std::begin;
using std::end;

return make_range(
make_sentinel_iterator(begin(s), end(s)),
make_sentinel_end(begin(s), end(s)));
auto b = begin(s);
auto e = end(s);
return make_range(make_sentinel_iterator(b, e), make_sentinel_end(b, e));
}

// Strictly evaluate end point in sentinel-terminated range and present as a range over
Expand All @@ -171,7 +170,6 @@ template <typename Seq>
auto strict_view(Seq&& s) {
using std::begin;
using std::end;

auto b = begin(s);
auto e = end(s);
return make_range(b, b==e? b: std::next(util::upto(b, e)));
Expand Down
13 changes: 6 additions & 7 deletions test/unit-distributed/test_distributed_for_each.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,9 @@ TEST(distributed_for_each, one_zero) {
for (int i = 0; i < rank; ++i) { data.push_back(rank); }

auto sample = [&](const util::range<int*>& range) {
const auto origin_rank = range.empty() ? 0 : range.front();

std::size_t origin_rank = range.empty() ? 0 : range.front();
EXPECT_EQ(origin_rank, range.size());
for (const auto& value: range) { EXPECT_EQ(value, origin_rank); }
for (std::size_t value: range) { EXPECT_EQ(value, origin_rank); }
++call_count;
};

Expand Down Expand Up @@ -71,14 +70,14 @@ TEST(distributed_for_each, multiple) {
auto sample = [&](const util::range<int*>& range_1,
const util::range<double*>& range_2,
const util::range<std::complex<double>*>& range_3) {
const auto origin_rank = range_1.empty() ? 0 : range_1.front();
std::size_t origin_rank = range_1.empty() ? 0 : range_1.front();

EXPECT_EQ(origin_rank + 1, range_1.size());
EXPECT_EQ(range_2.size(), 2 * range_1.size());
EXPECT_EQ(range_3.size(), 3 * range_1.size());
for (const auto& value: range_1) { EXPECT_EQ(value, origin_rank); }
for (const auto& value: range_2) { EXPECT_EQ(value, double(origin_rank)); }
for (const auto& value: range_3) { EXPECT_EQ(value, std::complex<double>(origin_rank)); }
for (std::size_t value: range_1) { EXPECT_EQ(value, origin_rank); }
for (auto value: range_2) { EXPECT_EQ(value, double(origin_rank)); }
for (auto value: range_3) { EXPECT_EQ(value, std::complex<double>(origin_rank)); }
++call_count;
};

Expand Down
8 changes: 4 additions & 4 deletions test/unit-distributed/test_network_generation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ TEST(network_generation, all) {
}

for (const auto& group: decomp.groups()) {
const auto num_dest = group.kind == cell_kind::spike_source ? 0 : 1;
for (const auto gid: group.gids) {
std::size_t num_dest = group.kind == cell_kind::spike_source ? 0 : 1;
for (std::size_t gid: group.gids) {
EXPECT_EQ(connections_by_dest[gid].size(), num_cells * num_dest);
}
}
Expand All @@ -141,7 +141,7 @@ TEST(network_generation, cable_only) {
const auto weight = 2.0;
const auto delay = 3.0;

const auto num_cells = 3 * num_ranks;
const std::size_t num_cells = 3 * num_ranks;

auto rec = network_test_recipe(num_cells, selection, weight, delay);

Expand All @@ -161,7 +161,7 @@ TEST(network_generation, cable_only) {
for (const auto gid: group.gids) {
// Only one third is a cable cell
EXPECT_EQ(connections_by_dest[gid].size(),
group.kind == cell_kind::cable ? num_cells / 3 : 0);
group.kind == cell_kind::cable ? num_cells / 3 : 0);
}
}
}
1 change: 1 addition & 0 deletions test/unit/common.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <utility>
#include <vector>
#include <algorithm>
#include <ostream>

#include <gtest/gtest.h>

Expand Down
64 changes: 28 additions & 36 deletions test/unit/test_range.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
#include "util/meta.hpp"
#include "util/range.hpp"
#include "util/rangeutil.hpp"
#include "util/sentinel.hpp"
#include "util/transform.hpp"

#include "common.hpp"
Expand Down Expand Up @@ -442,41 +441,34 @@ struct foo {
};

TEST(range, sort) {
char cstr[] = "howdy";

auto cstr_range = util::make_range(std::begin(cstr), null_terminated);

// Alas, no forward_iterator sort yet, so make a strict (non-sentinel)
// range to sort on below

// simple sort
util::sort(util::strict_view(cstr_range));
EXPECT_EQ("dhowy"s, cstr);

// reverse sort by transform c to -c
util::sort_by(util::strict_view(cstr_range), [](char c) { return -c; });
EXPECT_EQ("ywohd"s, cstr);

// stable sort: move capitals to front, numbers to back
auto rank = [](char c) {
return std::isupper(c)? 0: std::isdigit(c)? 2: 1;
};

char mixed[] = "t5hH4E3erLL2e1O";
auto mixed_range = util::make_range(std::begin(mixed), null_terminated);

util::stable_sort_by(util::strict_view(mixed_range), rank);
EXPECT_EQ("HELLOthere54321"s, mixed);


// sort with user-provided less comparison function

std::vector<foo> X = {{0, 5}, {1, 4}, {2, 3}, {3, 2}, {4, 1}, {5, 0}};

util::sort(X, [](const foo& l, const foo& r) {return l.y<r.y;});
EXPECT_EQ(X, (std::vector<foo>{{5, 0}, {4, 1}, {3, 2}, {2, 3}, {1, 4}, {0, 5}}));
util::sort(X, [](const foo& l, const foo& r) {return l.x<r.x;});
EXPECT_EQ(X, (std::vector<foo>{{0, 5}, {1, 4}, {2, 3}, {3, 2}, {4, 1}, {5, 0}}));
{
// simple sort
char cstr[] = "howdy";
util::sort(util::range_n(cstr, 5));
EXPECT_EQ("dhowy"s, cstr);
}
{
// reverse sort by transform c to -c
char cstr[] = "howdy";
util::sort_by(util::range_n(cstr, 5),
[](char c) { return -c; });
EXPECT_EQ("ywohd"s, cstr);
}
{
// stable sort: move capitals to front, numbers to back
char mixed[] = "t5hH4E3erLL2e1O";
util::stable_sort_by(util::strict_view(util::make_range(std::begin(mixed), null_terminated)),
[](char c) { return std::isupper(c)? 0: std::isdigit(c)? 2: 1; });
EXPECT_EQ("HELLOthere54321"s, mixed);
}
{
// sort with user-provided less comparison function
std::vector<foo> X = {{0, 5}, {1, 4}, {2, 3}, {3, 2}, {4, 1}, {5, 0}};
util::sort(X, [](const foo& l, const foo& r) {return l.y<r.y;});
EXPECT_EQ(X, (std::vector<foo>{{5, 0}, {4, 1}, {3, 2}, {2, 3}, {1, 4}, {0, 5}}));
util::sort(X, [](const foo& l, const foo& r) {return l.x<r.x;});
EXPECT_EQ(X, (std::vector<foo>{{0, 5}, {1, 4}, {2, 3}, {3, 2}, {4, 1}, {5, 0}}));
}
}

TEST(range, sum) {
Expand Down
4 changes: 2 additions & 2 deletions test/unit/test_simd.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -360,13 +360,13 @@ TYPED_TEST_P(simd_value, comparison) {
for (unsigned i = 0; i<nrounds; ++i) {
int cmp[N];
bool test[N];
simd a, b;
simd a = 0, b = 0;

fill_random(b, rng);

for (unsigned j = 0; j<N; ++j) {
cmp[j] = sgn(rng);
a[j] = b[j]+17*cmp[j];
a[j] = b[j] + 17*cmp[j];
}

mask gt = a>b;
Expand Down

0 comments on commit 9ba170a

Please sign in to comment.