Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#448 closed defect (fixed)

trunk (as of r3972) doesn't compile with clang++

Reported by: jinmei Owned by: jinmei
Priority: high Milestone:
Component: build system Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 0.5 Internal?: no

Description

We discussed this before. It's due to the exposure of asio definitions in asiolink tests.

Subtickets

Change History (7)

comment:1 Changed 9 years ago by jinmei

  • Owner changed from jreed to UnAssigned
  • Priority changed from major to critical
  • Status changed from new to reviewing

branches/trac448 is ready for review. Could someone review it?

This branch consists two independent sets of changes:

  • fix to asiolink tests: r4011
  • fix to testutils: r4012 and r4013

The diff is quite big, but it's largely because of copy-paste changes. The only non-trivial changes are (IMO) those in asiolink_unittest.cc to replace code using asio::* definitions.

This is the proposed changelog entry:

  139.?	[build]		jinmei
	Fixed build problems with clang++ in unit tests due to recent
	changes.  No behavior change. (Trac #448, svn rTBD)

I'm increasing the priority to "critical" as this should have been more serious if we had set up a buildbot with clang++.

comment:2 Changed 9 years ago by shane

  • Owner changed from UnAssigned to shane

comment:3 follow-up: Changed 9 years ago by shane

  • Owner changed from shane to jinmei

You learn a lot by reading code. I never knew SO_RCVTIMEO existed. Now I'm thinking of all kinds of possibilities. :) Sadly this appears to not work in Solaris, returning ENOPROTOOPT:

#define ENOPROTOOPT 99 /* Protocol not available */

This timeout could be reworked using a select()/poll()/whatever mechanism, or perhaps SIGALRM could be used. Alternately we skip these tests on Solaris.

Otherwise this looks good, and having the shocking result of being a patch that reduces lines of code!

comment:4 in reply to: ↑ 3 Changed 9 years ago by jinmei

  • Owner changed from jinmei to shane

Replying to shane:

You learn a lot by reading code. I never knew SO_RCVTIMEO existed. Now I'm thinking of all kinds of possibilities. :) Sadly this appears to not work in Solaris, returning ENOPROTOOPT:

#define ENOPROTOOPT 99 /* Protocol not available */

This timeout could be reworked using a select()/poll()/whatever mechanism, or perhaps SIGALRM could be used. Alternately we skip these tests on Solaris.

Otherwise this looks good, and having the shocking result of being a patch that reduces lines of code!

Thanks for the prompt review.

I've worked around the Solaris issue in r4055 and r4058. I chose a relatively simple workaround: just switch to non blocking recv() when RCVTIMEO fails with ENOPROTOOPT. If we still encounter the recv() failure due to the timing issue so often on Solaris, we'll then consider a more complete solution (using select/poll, or add some more ad hoc wait with usleep() when seeing ENOPROTOOPT, etc).

I also noticed some build errors on Solaris, which were fixed in r4054, r4056, r4059, r4061, and r4062.

Are these fixes okay and is the branch ready to merge?

comment:5 follow-up: Changed 9 years ago by shane

  • Add Hours to Ticket changed from 0.0 to 0.5
  • Owner changed from shane to jinmei
  • Total Hours changed from 0.0 to 0.5

Okay, I'm fine with the patches as they are, although I think we'll probably start getting errors from Solaris. But as you say, we can implement a more complete solution in that case!

comment:6 in reply to: ↑ 5 Changed 9 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed

Replying to shane:

Okay, I'm fine with the patches as they are, although I think we'll probably start getting errors from Solaris. But as you say, we can implement a more complete solution in that case!

Okay, thanks for the review. Committed to trunk, closing ticket.

comment:7 Changed 9 years ago by stephen

  • Milestone feature backlog item deleted

Milestone feature backlog item deleted

Note: See TracTickets for help on using tickets.