On Mon, Mar 17, 2025 at 3:30 PM Taylor Blau <me@xxxxxxxxxxxx> wrote: > > On Sun, Mar 16, 2025 at 06:42:01AM +0000, Elijah Newren via GitGitGadget wrote: > > We have roughly 566 assert() calls in our codebase (my grep might have > > picked up things that aren't actually assert() calls, but most appeared > > to be). All but 9 of them can be determined by gcc to be free of side > > effects with a clever redefine of assert() provided by Bruno De Fraine > > (from > > https://stackoverflow.com/questions/10593492/catching-assert-with-side-effects), > > who upon request has graciously placed his two-liner into the public > > domain without warranty of any kind. The current 9 assert() calls > > flagged by this clever redefinition of assert() appear to me to be free > > of side effects as well, but are too complicated for a compiler/linker > > to figure that since each assertion involves some kind of function call. > > Add a CI job which will find and report these possibly problematic > > assertions, and have the job suggest to the user that they replace these > > with BUG_IF_NOT() calls. > > Very nice, and thank you Bruno for placing your very clever assert() in > the public domain :-). > > I wonder if it might be useful to explain this in > Documentation/CodingGuidelines as a follow-up to this series. I was > thinking of a scenario where someone either writes a side-effecting > assert(), or a non-side-effecting one that is too complicated to prove > otherwise. > > If that person runs 'make test' locally, they might not see any > failures, but then be surprised when CI fails on the new step. It may be > worth mentioning that we have such a check, and that we expect all > assert() statements to be side effect-free, and that developers can > verify this by ci/check-unsafe-assertions.sh. The same could be said for coccinelle patches, hdr-check, check-pot, fuzz tests, asan/ubsan, GIT_TEST_SPLIT_INDEX, pedantic build, osx, vs. windows vs. linux, and perhaps others, which users won't catch on 'make test' locally but can result in failed CI builds and aren't mentioned in CodingGuidelines. I usually think of CodingGuidelines as being the place for documenting things that can't be tested in an automated fashion, and a brief mention that both cross platform and additional more thorough but non-default tests can go in SubmittingPatches. > But that may bring us into an assert() versus BUG_IF_NOT() debate, which > may be somewhat counterproductive, so I'm just as happy if you did > nothing here :-). :-)
