Audit all asserts to ensure they don't have side effects

#15188 (moved) fixed one instance, we should make sure there aren't more. Assignments are clearly bad, function calls are potentially bad. Anything else I'm looking for?

changed milestone to %Tor: 0.2.7.x-final

added component::core tor/tor milestone::Tor: 0.2.7.x-final priority::low resolution::fixed status::closed type::defect labels

A quick grep for tor_assert(.*( and manual parsing turned up with

src/common/compat_pthreads.c:281: tor_assert(0==pthread_attr_init(&attr_detached));
src/common/compat_pthreads.c:282: tor_assert(0==pthread_attr_setdetachstate(&attr_detached, 1));
src/or/rendservice.c:3323: tor_assert(!crypto_pk_generate_key(intro->intro_key));

yeah, tho we really want to look until the next semicolon. Also we should split any instances where we use a &&.

This is a cool thing to do, but let's remember it isn't critical: The tor source doesn't let you define NDEBUG, so we don't need to worry about assertions getting eliminated. This is a style issue, not a safety issue.

Still, it's worth fixing.

Trac:
Priority: normal to minor

I agree it's not critical, but please consider it regardless :) branch bug15211 in my repo

Trac:
Status: new to needs_review

Hmn. Is this actually making the code better? Second opinions welcome. I wonder if instead we shouldn't define a replacement for tor_assert that explicitly allows side-effects.

My two cents. This makes the code better for the following reasons thus I ack Sebastian patch.

1. Readability which is important as more and more contributors start looking/using the code. I myself doesn't expect function calls in an assert() because they can't be removed (not in the case of tor though). It's just not something you expect in C code imo.

2. Maintainability over time, keep the code consistent which often is the starting point for new contributors to look at the code to understand the coding style before submitting a patch (yes not always the case, I know).

I absolutely agree, this is not critical but the change is trivial and useful imo. Probably the change file could be "Coding style" instead of "bugfix" because no bug is fixed here.

I implemented this mostly because I need it for the feature where we're not using assertions for coverage builds

Ah, that's reasonable.

Merged, but turned some of the ints into const ints to make the code (IMO) even clearer. Leaving open since there are probably more of these?

Trac:
Status: needs_review to new

No I think I got all of them. I looked at:

• every multi-line assert individually
• every assert where we have an opening brace before ;
• every assert where we have assignment, including +=, *=, ++, etc (we have zero of these)

Thanks for merging and the const fix. Clearly I should work on my const-correctness

ok, neat! I'll close and we can reopen if we find any more.

Trac:
Resolution: N/A to fixed
Status: new to closed

closed

moved to tpo/core/tor#15211 (closed)