Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Polish CodeQL and do some chores #2255

Merged
merged 10 commits into from
Feb 22, 2024
8 changes: 4 additions & 4 deletions arbor/cable_cell_group.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -210,17 +210,17 @@ void run_samples(
fvm_probe_scratch& scratch)
{
const sample_size_type n_raw_per_sample = p.raw_handles.size();
sample_size_type n_sample = (sc.end_offset-sc.begin_offset)/n_raw_per_sample;
arb_assert((sc.end_offset-sc.begin_offset)==n_sample*n_raw_per_sample);
arb_assert((unsigned)n_raw_per_sample==p.weight.size());
sample_size_type n_sample = (sc.end_offset - sc.begin_offset)/n_raw_per_sample;
arb_assert((sc.end_offset - sc.begin_offset)==n_sample*n_raw_per_sample);
arb_assert((unsigned)n_raw_per_sample == p.weight.size());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these changes to make CodeQL happy, or just for readability?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Readability. I can get behind a*b + c for similarities with mathematical notation, but =, ==, and especially - tend to merge visually into one token. Or: Too similar to snake_case for my liking.
As much of the code, especially newer parts is in this style, I tend to adjust these expression in passing when editing files. Same for removing redundant headers, which my IDE picks up on these days.


auto& sample_ranges = std::get<std::vector<cable_sample_range>>(scratch);
sample_ranges.clear();
sample_records.clear();

auto& tmp = std::get<std::vector<double>>(scratch);
tmp.clear();
tmp.reserve(static_cast<std::size_t>(n_raw_per_sample)*n_sample);
tmp.reserve(n_raw_per_sample*n_sample);

for (sample_size_type j = 0; j<n_sample; ++j) {
auto offset = j*n_raw_per_sample+sc.begin_offset;
Expand Down
15 changes: 7 additions & 8 deletions arbor/communication/dry_run_context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,25 +47,24 @@ struct dry_run_context_impl {
void remote_ctrl_send_done() const {}
gathered_vector<cell_gid_type>
gather_gids(const std::vector<cell_gid_type>& local_gids) const {
using count_type = typename gathered_vector<cell_gid_type>::count_type;

count_type local_size = local_gids.size();
size_t local_size = local_gids.size();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::size_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and all other instance in that file.


std::vector<cell_gid_type> gathered_gids;
gathered_gids.reserve(size_t{local_size}*num_ranks_);
gathered_gids.reserve(local_size*num_ranks_);

for (count_type i = 0; i < num_ranks_; i++) {
for (size_t i = 0; i < num_ranks_; i++) {
util::append(gathered_gids, local_gids);
}

for (count_type i = 0; i < num_ranks_; i++) {
for (count_type j = i*local_size; j < (i+1)*local_size; j++){
for (size_t i = 0; i < num_ranks_; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 more std::size_ts

for (size_t j = i*local_size; j < (i+1)*local_size; j++){
gathered_gids[j] += num_cells_per_tile_*i;
}
}

using count_type = typename gathered_vector<cell_gid_type>::count_type;
std::vector<count_type> partition;
for (count_type i = 0; i <= num_ranks_; i++) {
for (size_t i = 0; i <= num_ranks_; i++) {
partition.push_back(static_cast<count_type>(i*local_size));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no point doing everything in size_t only to cast back down to count_type here; the alterations just make the potential bug more obscure. IMO the only correct resolution is to have an assert that checks that the product doesn't wrap and then to keep using count_type.

}

Expand Down
5 changes: 3 additions & 2 deletions arbor/include/arbor/util/hash_def.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,9 @@ namespace detail {
template <typename T>
inline constexpr std::size_t internal_hash(T&& data) {
using D = std::decay_t<T>;
constexpr std::size_t prime = 0x100000001b3;
constexpr std::size_t offset_basis = 0xcbf29ce484222325;
// Due to constexpr if we might never use these.
[[maybe_unused]] constexpr std::size_t prime = 0x100000001b3;
[[maybe_unused]] constexpr std::size_t offset_basis = 0xcbf29ce484222325;
static_assert(!std::is_pointer_v<D> || std::is_same_v<D, void*> || std::is_convertible_v<T, std::string_view>,
"Pointer types except void* will not be hashed.");
if constexpr (std::is_convertible_v<T, std::string_view>) {
Expand Down
12 changes: 8 additions & 4 deletions modcc/io/prefixbuf.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,14 @@

static void apply_indent_prefix(std::ios& s, int index) {
if (auto pbuf = dynamic_cast<prefixbuf*>(s.rdbuf())) {
indent_stack* stack_ptr = static_cast<indent_stack*>(s.pword(index));
size_t tabs = (!stack_ptr || stack_ptr->empty())? 0 : stack_ptr->top();
size_t tabwidth = s.iword(index);
pbuf->prefix = std::string(tabs*tabwidth, ' ');
if (auto ptr = static_cast<indent_stack*>(s.pword(index));
ptr && !ptr->empty()) {
unsigned n_tab = ptr->top();
Copy link
Contributor

@halfflat halfflat Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh! This is terribly misleading formatting.
Edit: what was so bad with unsigned tabs = (!stack_ptr || stack_ptr->empty())? 0: stack_ptr->top();?

Copy link
Contributor Author

@thorstenhater thorstenhater Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I knew you were going to love it. Isn't it beautiful how the second line of the condition and the first line of the body line up?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has a certain horrible charm to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it.

unsigned width = s.iword(index);
pbuf->prefix = std::string(n_tab*width, ' ');

Check failure

Code scanning / CodeQL

Multiplication result converted to larger type High

Multiplication result may overflow 'unsigned int' before it is converted to 'size_type'.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissed.

} else {
pbuf->prefix = std::string();
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions test/unit/test_sde.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -377,8 +377,8 @@ class sde_recipe: public simple_recipe_base {
// generic advance method used for all pertinent mechanisms
// first argument indicates the number of random variables
void advance_common(unsigned int n_rv, arb_mechanism_ppack* pp) {
const auto width = pp->width;
arb_value_type* ptr = archive_ptr->claim(std::size_t{width} * n_rv);
auto width = static_cast<size_t>(pp->width);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just size_t width = pp->width?

arb_value_type* ptr = archive_ptr->claim(width*n_rv);
Dismissed Show dismissed Hide dismissed
for (arb_size_type j=0; j<n_rv; ++j) {
for (arb_size_type i=0; i<width; ++i) {
ptr[j*width+i] = pp->random_numbers[j][i];
Expand Down