#32764 closed defect (implemented)

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

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points: 1
Parent ID: #29226 Points: 1
Reviewer: teor Sponsor:

Description

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.

Child Tickets

Change History (15)

comment:1 Changed 10 months ago by nickm

Actual Points: 1

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.

comment:2 Changed 10 months ago by nickm

Status: assignedneeds_review

comment:3 in reply to:  1 Changed 10 months ago by catalyst

Status: needs_reviewneeds_revision

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.

comment:4 Changed 10 months ago by nickm

Status: needs_revisionneeds_review

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

comment:5 Changed 10 months ago by teor

Status: needs_reviewneeds_revision

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.

comment:6 Changed 10 months ago by teor

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

comment:7 Changed 10 months ago by nickm

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.

comment:8 Changed 10 months ago by nickm

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

comment:9 Changed 10 months ago by nickm

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.

comment:10 Changed 10 months ago by nickm

Status: needs_revisionneeds_review

(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.)

comment:11 in reply to:  1 Changed 10 months ago by teor

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.

comment:12 Changed 10 months ago by nickm

Reviewer: teor

setting teor as current reviewer on this.

comment:13 in reply to:  5 Changed 10 months ago by teor

Status: needs_reviewmerge_ready

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.)

comment:14 Changed 10 months ago by nickm

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

comment:15 Changed 10 months ago by nickm

Resolution: implemented
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.