Opened 2 months ago

Closed 6 weeks ago

#23149 closed defect (implemented)

Refactor buffer.c: split and rename functions.

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-22
Cc: Actual Points: .2
Parent ID: #22342 Points:
Reviewer: Sponsor:

Description


Child Tickets

Change History (15)

comment:1 Changed 2 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:2 Changed 2 months ago by nickm

Please see branch refactor_buffers_api in my public repository.

comment:3 Changed 2 months ago by nickm

Actual Points: .2
Status: acceptedneeds_review

comment:4 Changed 2 months ago by nickm

Keywords: review-group-22 added

comment:5 Changed 2 months ago by dgoulet

Status: needs_reviewneeds_information

The git diff I get is:

37 files changed, 2676 insertions(+), 2470 deletions(-)

If it's the case, maybe a Gitlab MR ?

Last edited 2 months ago by dgoulet (previous) (diff)

comment:6 Changed 8 weeks ago by nickm

That's mostly code movement, but sure! Here you go:

https://oniongit.eu/nickm/tor/merge_requests/5

comment:7 Changed 7 weeks ago by nickm

Status: needs_informationneeds_review

comment:8 Changed 7 weeks ago by catalyst

Reviewer: catalyst

comment:9 Changed 7 weeks ago by catalyst

Status: needs_reviewneeds_revision

Still looking, but there are currently merge conflicts with master.

comment:10 Changed 7 weeks ago by catalyst

gcc Travis builds failed (after fixing up two unresolved merge conflicts in test_buffers.c):

In file included from src/common/buffers_tls.c:10:0:
src/common/buffers.h:70:27: error: ‘preferred_chunk_size’ declared ‘static’ but never defined [-Werror=unused-function]
 ATTR_UNUSED STATIC size_t preferred_chunk_size(size_t target);
                           ^
cc1: all warnings being treated as errors
make: *** [src/common/buffers_tls.o] Error 1
make: *** Waiting for unfinished jobs....

It looks like buffers_tls.c defines BUFFERS_PRIVATE but doesn't provide a STATIC definition of preferred_chunk_size(). (though it would probably cause duplicate definitions when built with -DTOR_UNIT_TESTS)

I'm not sure what's best here; maybe a different macro to protect the STATIC declaration in buffers.h?

edit: typo

Last edited 7 weeks ago by catalyst (previous) (diff)

comment:11 Changed 7 weeks ago by catalyst

Reviewer: catalyst
Status: needs_revisionneeds_review

Proposed fixups in the nickm/refactor_buffers_api branch in my github repo.

comment:12 Changed 7 weeks ago by catalyst

Otherwise, the changes look good to me.

comment:13 Changed 6 weeks ago by nickm

I've tried, but I can't reproduce that warning above. I would have thought that the ATTR_UNUSED would have suppressed that warning. Maybe it depends on GCC version? I'm going to try merging without that fix, to see where it breaks, and look for a better way to fix it.

comment:14 Changed 6 weeks ago by nickm

Oh, never mind. It's GCC only, and clang doesn't object.

comment:15 Changed 6 weeks ago by nickm

Resolution: implemented
Status: needs_reviewclosed

I took a simple approach in c0b9f594b65f410cf219673d53226ed4eeeedc19: I decided to make the function not static any more.

Otherwise, changes look fine -- merged!

Note: See TracTickets for help on using tickets.