fuzz: silence a compiler warning about unused CBanEntry comparator #22557

pull vasild wants to merge 1 commits into bitcoin:master from vasild:hide_ban_entry_compare changing 1 files +3 −2

vasild commented at 4:03 PM on July 26, 2021: member

test/fuzz/banman.cpp:35:13: warning: unused function 'operator==' [-Wunused-function]
static bool operator==(const CBanEntry& lhs, const CBanEntry& rhs)
            ^
1 warning generated.

See #22517 (comment)

DrahtBot added the label Tests on Jul 26, 2021
hebasto commented at 6:03 PM on July 26, 2021: member

See #22517 (comment)

Actually, I mentioned it expecting no one will silent a temporary warning :smile:
jarolrod commented at 6:33 PM on July 26, 2021: member
No strong opinion on this. It will be interesting to see the response to this.

Do we want to have a policy of silencing temporary warnings when their is a clear plan that will silence them? --- this warning will be silenced when the fix for the banman fuzz test is implemented.

On the other hand, the argument could be made that #22517 should not have allowed this warning to come up and this PR can be seen as a follow-up to that PR. A followup which finishes properly temporarily disabling the assert.

Tested that this silences the warning on Ubuntu 20.04: master:
```
$ make 2>&1 > /dev/null | grep -o "Wunused-function" | wc -l

1
```
pr:
```
$ make 2>&1 > /dev/null | grep -o "Wunused-function" | wc -l

0
```
vasild commented at 7:09 AM on July 27, 2021: member

This warning disturbs developers' workflow (compilation with -Werror fails).

It does not matter whether it is temporary, semi-temporary, permanently-temporary, temo-low or should just be painted in purple color.
MarcoFalke commented at 8:23 AM on July 27, 2021: member

cr ACK 83dd35260b30eaedc8822326f75c037d5e51f883

Sorry didn't see the warning in my patch

fuzz: silence a compiler warning about unused CBanEntry comparator

```
test/fuzz/banman.cpp:35:13: warning: unused function 'operator==' [-Wunused-function]
static bool operator==(const CBanEntry& lhs, const CBanEntry& rhs)
            ^
1 warning generated.
```

787296eb67

in src/test/fuzz/banman.cpp:119 in 83dd35260b outdated

 114 | @@ -109,8 +115,9 @@ FUZZ_TARGET_INIT(banman, initialize_banman)
 115 |              BanMan ban_man_read{banlist_file, /* client_interface */ nullptr, /* default_ban_time */ 0};
 116 |              banmap_t banmap_read;
 117 |              ban_man_read.GetBanned(banmap_read);
 118 | -            // Temporarily disabled to allow the remainder of the fuzz test to run while a fix is being worked on:
 119 | -            // assert(banmap == banmap_read);
 120 | +#ifdef ENABLE_BANMAN_COMPARE
 121 | +            assert(banmap == banmap_read);

MarcoFalke commented at 9:23 AM on July 27, 2021:

nit: wouldn't it be easier to just:

            (void)(banmap == banmap_read);

vasild commented at 12:00 PM on July 27, 2021:

Yes, that is shorter, thanks!

vasild force-pushed on Jul 27, 2021
vasild commented at 11:59 AM on July 27, 2021: member

83dd35260b...787296eb67: take review suggestion
hebasto approved
hebasto commented at 12:05 PM on July 27, 2021: member

ACK 787296eb6744b15ab693c053e4030ff68dfc95e0
laanwj commented at 12:55 PM on July 27, 2021: member

I don't think we need to follow every temporary change with a change to clean up warnings. If there is intent to use this function soon, I do not see the point of merging an intermediate PR that removes it just to be reintroduced.

Edit: oh, you didn't remove it. This seems fine to me (though still I don't think we should be too busybody about this).
MarcoFalke commented at 3:50 PM on July 27, 2021: member

cr ACK 787296eb6744b15ab693c053e4030ff68dfc95e0

again my fault for not noticing the compiler warning in the initial patch
practicalswift commented at 3:59 PM on July 27, 2021: contributor

cr ACK 787296eb6744b15ab693c053e4030ff68dfc95e0
fanquake merged this on Jul 28, 2021
fanquake closed this on Jul 28, 2021
vasild deleted the branch on Jul 28, 2021
sidhujag referenced this in commit 28706338a4 on Jul 28, 2021
hebasto referenced this in commit 9214541e00 on Aug 6, 2021
hebasto commented at 3:54 PM on August 6, 2021: member

~Backported in #22629.~

UPDATE: #22629 (comment)
hebasto referenced this in commit 482ab6d3bb on Aug 6, 2021
hebasto referenced this in commit 20c1331b84 on Aug 7, 2021
hebasto referenced this in commit c427a01e01 on Aug 9, 2021
hebasto referenced this in commit b5968dff37 on Aug 9, 2021
hebasto referenced this in commit 7e58f5cf1a on Aug 19, 2021
hebasto referenced this in commit e3d207d6ee on Aug 20, 2021
hebasto referenced this in commit f6c4c79705 on Aug 20, 2021
DrahtBot locked this on Aug 20, 2022

Contributors

Labels

Linked (view graph)

#22517 fuzz: Temporarily disable failing assert in banman fuzz test