Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#4893 closed defect (implemented)

Rename inconsistent function and type names

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

By convention:

  • Types do not have "env" or "object`" or whatever in them.
  • The primary function to free a foo is called foo_free(), not foo_free_env().
  • The primary function that allocates and returns a newly allocated foo is called foo_new(), not foo_create() or foo_new_env().

We violate these rules, though, with a fair number of longstanding types and identifiers. How annoying! Let's fix that.

This will make some pending unmerged code get merge conflicts or need rewrites, but now that we're moving towards the end of the 0.2.3.x release cycle, it's the perfect time to do that.

Child Tickets

Attachments (1)

renamer.pl (909 bytes) - added by nickm 8 years ago.
Perl identifier renamer

Download all attachments as: .zip

Change History (7)

Changed 8 years ago by nickm

Attachment: renamer.pl added

Perl identifier renamer

comment:1 Changed 8 years ago by nickm

Status: newneeds_review

I've attached a perl script that I propose to run on src/*/*.[ch] .

(Doing diff would be silly before we're ready to merge, though I'm happy to try if people want.)

Opinions wanted!

comment:2 Changed 8 years ago by Sebastian

Do we have this convention documented anywhere? I was never too bothered with the 'env' stuff. It seemed to set up the crypto environment, so I treated it as being descriptive of what it does.

That said, the script doesn't introduce breakage of anything per se, and if you feel these are an improvement I also don't object to the new names.

(To illustrate what I meant with the first sentence, with the rename script this:

/* environment setup */
crypto_pk_env_t *crypto_new_pk_env(void);
void crypto_free_pk_env(crypto_pk_env_t *env);

becomes this:

/* environment setup */
crypto_pk_t *crypto_pk_new(void);
void crypto_pk_free(crypto_pk_t *env);

)

comment:3 Changed 8 years ago by nickm

Do we have this convention documented anywhere?

Not explicitly; IMO it's implicit in all the other names we have.

My objection to "env" is not that it's inaccurate but that it's meaningless. Everything can be construed as an environment, potentially. A policy could be a policy_env_t; a connection could be a connection_env_t; a circuit could be a circuit_env_t; etc.

It's the same argument for why you should name your objects things like circuit_t rather than circuit_object_t.

comment:4 Changed 8 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Applied after review from arma

comment:5 Changed 7 years ago by nickm

Keywords: tor-client added

comment:6 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.