Opened 6 years ago

Closed 6 years ago

#11633 closed defect (fixed)

Quench a few warnings found with gcc-4.2 and clang

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

Description

On running configure --enable-gcc-warnings with gcc-4.2 on OpenBSD, as well as clang 3.3, has picked up a few things:

  • gcc-4.2 complains about discarding constness with TO_ORIGIN_CIRCUIT and TO_OR_CIRCUIT.
  • clang complains with -Wshorten-64-to-32 about implicit conversions between time_t and long, and OpenBSD has time_t as an int32_t.

The attached patches add a const version of TO_ORIGIN_CIRCUIT and TO_OR_CIRCUIT, and add explicit casts between long and time_t.

Child Tickets

Change History (10)

comment:1 Changed 6 years ago by nickm

Keywords: tor-client build added
Status: newneeds_revision

Thanks for the patches!

Should the CONST_TO_* functions return a const pointer?

I worry about the time_t patch on platforms like 32-bit NetBSD where long is 32 bit but time_t is 64-bit. On those platforms, it's safe to do:

  time_t now = time(NULL);
  long one_hour = 3600;
  time_t in_an_hour = now + one_hour;

but it isn't safe to do:

  time_t now = time(NULL);
  long one_hour = 3600;
  time_t in_an_hour = (time_t)((long)now + one_hour);

In other words, the code ought to handle sizeof(time_t) < sizeof(long) [the OpenBSD case] as well as sizeof(time_t) > sizeof(long) [the NetBSD case].

comment:2 Changed 6 years ago by _x3j11

I wasn't getting warnings without making the CONST_TO_* versions return const, but I think it's probably a good idea to do so anyway. I've updated the first patch.

I will have a think about the best way to address the NetBSD case (the second patch needs fixing anyway as it wasn't split correctly).

comment:3 Changed 6 years ago by nickm

Merged the 0001 patch.

comment:4 Changed 6 years ago by nickm

(as 88679aa53f81ed3b537f214094faa96f4e9a395d with changes file as acc7623420139d13377ac441e8d5c5f468cf95c5)

comment:5 in reply to:  2 Changed 6 years ago by nickm

Replying to _x3j11:

I will have a think about the best way to address the NetBSD case (the second patch needs fixing anyway as it wasn't split correctly).

So, most of the time_t patch is casting timeval.tv_sec to time_t before passing it to something that needs a time_t. That doesn't need a change.

For the 'add' cases, one option is to change the interval types to 'int'. We require that int be at least 32 bits, and I don't think we need any >68-year expiry times. But that might be a big patch; I'm not sure.

comment:6 Changed 6 years ago by _x3j11

Status: needs_revisionneeds_review

I've taken the approach to use the integer coercion rules (ie., the upcasts aren't strictly necessary after all) to do this, and casting to time_t right at the end. This should be correct.

comment:7 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Yes, this looks right. Merged!

Note: See TracTickets for help on using tickets.