Solve code issues that block running clang-format on our code.

There are some aspects of our code that confuse clang-format, or produce bad output. In nearly all cases, this is a sign of bad code.

changed milestone to %Tor: 0.4.3.x-final

Trac:
Parent Ticket: #29226 (moved)

added actualpoints::1 component::core tor/tor milestone::Tor: 0.4.3.x-final owner::nickm parent::29226 points::1 priority::medium resolution::implemented reviewer::teor severity::normal status::closed type::defect labels

See branch pre_formatter_cleanups, with PR at https://github.com/torproject/tor/pull/1610

I'll put this in needs_review once CI passes.

Trac:
Actualpoints: N/A to 1

Trac:
Status: assigned to needs_review

Replying to nickm:

See branch pre_formatter_cleanups, with PR at https://github.com/torproject/tor/pull/1610

I'll put this in needs_review once CI passes. Thanks! I skimmed it. It mostly looks good, but I didn't do extensive verification. I commented on the pull request about a few places where a now-undefined macro remained referenced.

Trac:
Status: needs_review to needs_revision

Good catch, and thanks for the super quick response! I've pushed a fixup commit there.

Trac:
Status: needs_revision to needs_review

I think options_validate_dirauth_mode(), set_server_advertised(), options_validate_server_transport() and options_validate_relay_mode() all introduce similar dependencies to authdir_mode_v3(). (Mostly on or_options_t.)

But they only add those dependencies in disable relay/dirauth mode, so you might not have seen any compilation issues after header re-ordering?

I'd like to see a test branch with these changes, and the header re-ordering, so that we can check all our standard builds in CI.

Trac:
Status: needs_review to needs_revision

#32798 (moved) is a comprehensive solution to this issue: compile every header by itself as part of our tests.

I tried building with modules disabled, and didn't hit the errors you expect. I've made a test branch demo_clang_format_20191218 to try it out on CI, as you suggest above.

Test PR at https://github.com/torproject/tor/pull/1617 (not to actually merge).

At first glance at the CI, it appears that the compilation is succeeding, but check-spaces is failing (as expected). There are also some coccinelle warnings, which are not what I had expected.

But from a "does it compile" POV I think maybe we can call the pre_formatter_cleanups branch tested, perhaps.

(Can we merge the "pre_formatter_cleanups" branch now? It does in fact do what we need, though we may want other changes down the line as well.)

Trac:
Status: needs_revision to needs_review

Replying to nickm:

See branch pre_formatter_cleanups, with PR at https://github.com/torproject/tor/pull/1610

I'll put this in needs_review once CI passes.

This is the branch we're reviewing and merging for this ticket.

setting teor as current reviewer on this.

Trac:
Reviewer: N/A to teor

These changes seem fine to me. GitHub is showing some conflicts, I'm happy for the merger to fix them up when they merge,

The following inline header functions depend on some members of or_options_t, which is a dependency we don't need:

• options_validate_dirauth_mode()
• options_validate_server_transport()
• options_validate_relay_mode() And the dependency only exists when the relay or dirauth modules are disabled.

So I'd like to open another ticket to remove that dependency. (We could add stub C files that are only compiled when relay/dirauth mode is disabled.)

Trac:
Status: needs_review to merge_ready

Thank you for the review! Squashed and merged this branch.

Trac:
Status: merge_ready to closed
Resolution: N/A to implemented

closed

changed time estimate to 8h