Skip to content

Fix warnings from Wall Wextra Wpedantic#258

Merged
hainest merged 98 commits intomasterfrom
thaines/wall_wextra_wpedantic
Apr 16, 2025
Merged

Fix warnings from Wall Wextra Wpedantic#258
hainest merged 98 commits intomasterfrom
thaines/wall_wextra_wpedantic

Conversation

@hainest
Copy link
Contributor

@hainest hainest commented Apr 3, 2025

No description provided.

@hainest hainest self-assigned this Apr 3, 2025
@hainest hainest force-pushed the thaines/wall_wextra_wpedantic branch 2 times, most recently from 58a4c40 to b413a90 Compare April 4, 2025 18:29
@hainest hainest force-pushed the thaines/wall_wextra_wpedantic branch 2 times, most recently from 650a842 to d4e6881 Compare April 4, 2025 20:59
@hainest hainest marked this pull request as ready for review April 6, 2025 19:26
@hainest hainest requested review from bbiiggppiigg and kupsch April 6, 2025 19:26
num_breakpoints_hit = 0;
hit_counts.clear();
memset(indexes, 0, sizeof(indexes));
memset(bp_addrs, 0, sizeof(bp_addrs));
Copy link
Member

Choose a reason for hiding this comment

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

If on_breakpoint can be called multiple times, then I assume the memset should be kept?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

std::pair isn't trivially constructible, so using memset is UB. Values in indexes are only used immediately after writing them, so the memset was always unneeded.

indexes[cur_index].first = j;
indexes[cur_index].second = k;
bps[j][k]->setData(indexes+cur_index);

The mixture of bracket operator and C-style array-plus-index is...

Copy link
Member

@bbiiggppiigg bbiiggppiigg Apr 9, 2025

Choose a reason for hiding this comment

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

What about the memset for bp_addrs?
It is an array of Dyninst::Address.
It is defined before use in executeTest.
But it is also checked in on_breakpoint L:107.

As for the mixture of C-style array-plus-index, it seems like setData is expecting a void *, so that seems valid to me.
Unless it should be something like setData(&indexes[cur_index])?

Copy link
Contributor Author

@hainest hainest Apr 9, 2025

Choose a reason for hiding this comment

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

What about the memset for bp_addrs? It is an array of Dyninst::Address. It is defined before use in executeTest. But it is also checked in on_breakpoint L:107.

Dyninst::Address is typedef'd to a primitive type, so that one is fine. I think it's also ok to convert T[][] to void*. I'd have to double-check.

As for the mixture of C-style array-plus-index, it seems like setData is expecting a void *, so that seems valid to me. Unless it should be something like setData(&indexes[cur_index])?

It's valid, but there's no need to write assembly in C++. It would be more clearly written &indexes[cur_index], as you pointed out.

@hainest hainest force-pushed the thaines/wall_wextra_wpedantic branch from d4e6881 to cb7ab0d Compare April 15, 2025 22:48
@hainest hainest merged commit d44858d into master Apr 16, 2025
1 check passed
@hainest hainest deleted the thaines/wall_wextra_wpedantic branch April 16, 2025 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants