Opened 8 years ago

Closed 8 years ago

Last modified 17 months ago

#365 closed enhancement (fixed)

clang++ support

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: y2 12 month milestone
Component: build system Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description


Subtickets

Change History (11)

comment:1 follow-up: Changed 8 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from new to reviewing

branches/trac365 is ready for review.

Notes:

  • older versions of boost doesn't explicitly support clang++, so I simply (and implicitly) required boost >= 1.44. In order to support build environments where two or more versions of boost are available (and only a part of them is new enough), I chose to extract the header file specification (-I<path_to_boost>) into separate configure/automake variables and made sure the specified path is preferred.
  • most of our code was cleanly built with minor adjustments, but ASIO triggered a warning that can't be disabled runtime, so I ended up turning off -Werror for the part of the code that needs to include ASIO. I've reported this issue on the asio-users ML: http://sourceforge.net/mailarchive/forum.php?thread_name=m27hhro1fq.wl%25jinmei%40isc.org&forum_name=asio-users Hopefully a future version of ASIO will fix it.
  • clang++ didn't like the following pattern of tests:
        EXPECT_EQ(false, <expression>);
    
    and I had to change these to:
        EXPECT_FALSE(<expression>);
    
    I didn't dig into why, but clang++ probably handles "false" in some tricky way. I didn't change the "true" patterns because it didn't trigger an error and when we can use EXPECT_EQ it would be better than EXPECT_TRUE.

The proposed changelog entry is as follows:

  106.?	[build]		jinmei
	Supported clang++.  Note: Boost >= 1.44 is required.
	(Trac #365, svn rTBD)

comment:2 Changed 8 years ago by jinmei

I've added one more change (r3164). It's not directly related to this ticket, but I noticed the sample benchmark didn't work as expected with optimized code produced by clang++, so I fixed it in this ticket.

comment:3 follow-up: Changed 8 years ago by jreed

Is the EXPECT_FALSE the required way for us to do this for now? Style guide entry? Or is this something to report to googletest developers?

Also it may be useful for some of the commit messages to explain why change was made versus just saying for clagg+ support. But maybe the changes are too obvious for others (like struct to class) to explain why it didn't matter with g++ or sunstudio.

comment:4 in reply to: ↑ 1 ; follow-up: Changed 8 years ago by each

  • older versions of boost doesn't explicitly support clang++, so I simply (and implicitly) required boost >= 1.44

Just to make sure... that requirement only pertains if building with clang++, right? Because 1.44 is very recent and most OS distributions won't likely have it yet.

I didn't dig into why, but clang++ probably handles "false" in some tricky way. I didn't change the "true" patterns because it didn't trigger an error and when we can use EXPECT_EQ it would be better than EXPECT_TRUE.

I don't understand this last point. Why is it better to use EXPECT_EQ() when possible? EXPECT_TRUE() is simpler and clearer.

comment:5 in reply to: ↑ 4 Changed 8 years ago by jinmei

Replying to each:

  • older versions of boost doesn't explicitly support clang++, so I simply (and implicitly) required boost >= 1.44

Just to make sure... that requirement only pertains if building with clang++, right?

Right.

Because 1.44 is very recent and most OS distributions won't likely have it yet.

Right, and that's exactly why I introduced the additional -I<path_to_boost>.

I didn't dig into why, but clang++ probably handles "false" in some tricky way. I didn't change the "true" patterns because it didn't trigger an error and when we can use EXPECT_EQ it would be better than EXPECT_TRUE.

I don't understand this last point. Why is it better to use EXPECT_EQ() when possible? EXPECT_TRUE() is simpler and clearer.

On second thought, I think you're right. I was thinking cases like

    EXPECT_TRUE(<exp1> == <exp2>); // should be EXPECT_EQ(<exp1>, <exp2>)

but it doesn't make (much) sense to keep the awkward style of

    EXPECT_EQ(true, <expression>);

So I've chanded the "true" patterns throughout the code tree as well. It's not a requirement for the clang++ support, but making both (true/false) patterns consistent in the same branch would be nice. It's r3197.

comment:6 in reply to: ↑ 3 Changed 8 years ago by jinmei

Replying to jreed:

Is the EXPECT_FALSE the required way for us to do this for now?

At least for supporting clang++, yes. Otherwise the test code won't compile.

Style guide entry?

It's probably not worth an explit guide, because the revised pattern is actually more natural, and in the case of "false" a buildbot with clang++ will report the breakage. (See also my previos comment, response to Evan's comment)

Or is this something to report to googletest developers?

Probably not for the same reason.

Also it may be useful for some of the commit messages to explain why change was made versus just saying for clagg+ support. But maybe the changes are too obvious for others (like struct to class) to explain why it didn't matter with g++ or sunstudio.

I see your point. It's a more general issue of which level of details we should add to commit logs though.

comment:7 Changed 8 years ago by jinmei

What's the current status of review for this patch?

It seems it will take time to get a buildbot environment for clang++, but I don't think it makes sense to defer the integration of the patch for that reason. The changes should be basically trivial and shouldn't disrupt exisiting environments.

Some people have made comments on the patch, and I've responded to/addressed them. Can I now merge it to trunk or should I wait for more complete review? Again, the change should be quite trivial so I basically don't think it makes sense to wait for too long. (And I'd like to use it, integrated to trunk if possible, on my personal server for some reason, hence this message)

comment:8 follow-up: Changed 8 years ago by jelte

  • Owner changed from UnAssigned to jinmei

Should we also print the compiler we chose in the configure overview output?

About the -Wno-error, in trac327, all asio code should have been localized to one location (we ran into unused warnings there as well), I'm assuming this is a similar problem, so if that is merged, we *should* be able to revert the changes for asio-specific problems here. As it stands I'm fine with them.

There's a few tests within specific rdata tests where EXPECT_TRUE(a > b) is used now, there is also an EXPECT_GT that should test the > operator.

I can confirm that it still works with gcc, but I have no working clang version at this moment (the older version i have crashes, and the latest svn version does not compile here), so I cannot confirm whether it works for me too. However, I see no problems with the changes, and except for the trivial comments above, I think it can be merged.

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

Replying to jelte:

Should we also print the compiler we chose in the configure overview output?

Good suggestion. Done in r3381.

About the -Wno-error, in trac327, all asio code should have been localized to one location (we ran into unused warnings there as well), I'm assuming this is a similar problem, so if that is merged, we *should* be able to revert the changes for asio-specific problems here.

Right.

There's a few tests within specific rdata tests where EXPECT_TRUE(a > b) is used now, there is also an EXPECT_GT that should test the > operator.

That makes sense, but it's beyond the scope of this ticket. I'll create a separate ticket for this.

I can confirm that it still works with gcc, but I have no working clang version at this moment (the older version i have crashes, and the latest svn version does not compile here), so I cannot confirm whether it works for me too. However, I see no problems with the changes, and except for the trivial comments above, I think it can be merged.

Okay, thanks for the review. I'll hold for a while due to the additional change (r3381) but unless I hear something I'll soon merge it to trunk.

comment:10 Changed 8 years ago by jinmei

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

merged to trunk, closing ticket.

comment:11 Changed 17 months ago by mtravis

  • Defect Severity set to N/A
  • Sub-Project set to DHCP

Here is the wiki link for ASIO

Note: See TracTickets for help on using tickets.