Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

clang: disable RTTI support with cxx_no_rtti_flags case for parity with g++ #2148

Closed
wants to merge 1 commit into from
Closed

clang: disable RTTI support with cxx_no_rtti_flags case for parity with g++ #2148

wants to merge 1 commit into from

Conversation

ngie-eign
Copy link
Contributor

@ngie-eign ngie-eign commented Feb 21, 2019 •
edited

Add cxx_no_rtti_flags for clang in order to test the case with RTTI off,
similar to g++.

This increases test coverage with the non-RTTI case with clang. This something
I used in when investigating test failures on FreeBSD, as the tests that rely
are failing with googletest 1.8.1 on the OS/platform, as described in
issue #2172.

Signed-off-by: Enji Cooper yaneurabeya@gmail.com

@ngie-eign ngie-eign force-pushed the clang-add-explicit-c++11-rtti-flag branch from 0ba1c6a to c6c9bd0 Compare February 21, 2019 20:13
@gennadiycivil
Copy link
Contributor

@ngie-eign could you please rebase/resolve conflicts

@ngie-eign ngie-eign force-pushed the clang-add-explicit-c++11-rtti-flag branch from c6c9bd0 to afdb3e2 Compare February 25, 2019 17:52
@ngie-eign
Copy link
Contributor Author

@ngie-eign could you please rebase/resolve conflicts

@gennadiycivil: done!

@EricWF
Copy link

EricWF commented Mar 6, 2019

How do we turn on C++11 today?

The fact we failed to set cxx_no_rtti_flags with Clang seems like a bug? Do we not have a test that catches it?

@gennadiycivil
Copy link
Contributor

@EricWF C++ 11 set. Some set in travis configs, some elsewhere, it is generally not well organized.
Given that CMake support is community-based this is probably an artifact of organic growth.

@EricWF
Copy link

EricWF commented Mar 7, 2019

@gennadiycivil It looks like we're using CMAKE_CXX_STANDARD to specify the dialect. I believe this is better practice than passing -std=c++11 as a flag.

@ngie-eign
Copy link
Contributor Author

@gennadiycivil It looks like we're using CMAKE_CXX_STANDARD to specify the dialect. I believe this is better practice than passing -std=c++11 as a flag.

Yes. I'll remove that explicit setting.

@ngie-eign ngie-eign mentioned this pull request Mar 9, 2019
Open
@gennadiycivil
Copy link
Contributor

Ping?

@ngie-eign ngie-eign force-pushed the clang-add-explicit-c++11-rtti-flag branch 2 times, most recently from 6dd1559 to 15a6eb3 Compare March 21, 2019 07:44
@ngie-eign
Copy link
Contributor Author

@gennadiycivil: pong :).

@ngie-eign ngie-eign force-pushed the clang-add-explicit-c++11-rtti-flag branch from 15a6eb3 to 3443810 Compare March 27, 2019 17:47
@ngie-eign
Copy link
Contributor Author

@gennadiycivil: ping?

@ngie-eign ngie-eign force-pushed the clang-add-explicit-c++11-rtti-flag branch 2 times, most recently from 28c9d9b to 07f1af0 Compare April 1, 2019 19:31
@ngie-eign
Copy link
Contributor Author

ngie-eign commented Apr 3, 2019 •
edited

@EricWF, @gennadiycivil: ping?

@ngie-eign ngie-eign force-pushed the clang-add-explicit-c++11-rtti-flag branch 3 times, most recently from 3443810 to 7475ba5 Compare April 4, 2019 04:12
@ngie-eign ngie-eign changed the title clang: explicitly enable/disable RTTI/C++11 support with the compiler clang: explicitly enable/disable RTTI support with the compiler for parity with g++ Apr 4, 2019
@EricWF
Copy link

EricWF commented Apr 4, 2019

I still don't understand why we weed to pass -frtti explicitly. We should never have to do that. RTTI should already be enabled by default.

@gennadiycivil
Copy link
Contributor

I edited the comment a bit to just leave rtti , as C++11 is no longer in scope...
Could you please explain a bit more , isnt RTTI is enabled by default with clang? Why do we have to explicitly enable it.
I can see how we may want to have a disable flag, yes....

Please advise?

Thanks again
G

@ngie-eign
Copy link
Contributor Author

ngie-eign commented Apr 4, 2019 •
edited

@EricWF: the concern I have is that downstream projects might disable RTTI in their builds, resulting in unexpected behavior when compiling with clang, or the default may change due to an upstream change or local modification. If either of these things happen, googletest may be compiled improperly, resulting in confusing behavior and test results, forcing one or more engineers to have to analyze why googletest isn’t functioning as expected.

@gennadiycivil
Copy link
Contributor

@ngie-eign I am not sure I agree with this premise. I don't believe googletest can presume to know what the downstream client means to when they choose disable RTTI . If the theoretical "they" chose to do this there must have been a reason.
Why do we want to force anything?

@ngie-eign
clang: disable RTTI support with cxx_no_rtti_flags case for parity …
6d393d7 
…with g++

Add `cxx_no_rtti_flags` for clang in order to test the case with RTTI off,
similar to g++.

This increases test coverage with the non-RTTI case with clang. This something
I used in when investigating test failures on FreeBSD, as the tests that rely
are failing with googletest 1.8.1 on the OS/platform, as described in
issue google#2172.

Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>
@ngie-eign ngie-eign force-pushed the clang-add-explicit-c++11-rtti-flag branch from 7475ba5 to 6d393d7 Compare April 4, 2019 18:33
@ngie-eign ngie-eign changed the title clang: explicitly enable/disable RTTI support with the compiler for parity with g++ clang: disable RTTI support with cxx_no_rtti_flags case for parity with g++ Apr 4, 2019
@ngie-eign
Copy link
Contributor Author

@ngie-eign I am not sure I agree with this premise. I don't believe googletest can presume to know what the downstream client means to when they choose disable RTTI . If the theoretical "they" chose to do this there must have been a reason.
Why do we want to force anything?

Ok, I've walked by the changes to assume the implicit -frtti being on behavior and have edited the commit log, etc to note this.

gennadiycivil added a commit that referenced this pull request Apr 5, 2019
@gennadiycivil
Merge pull request #2148 from ngie-eign:clang-add-explicit-c++11-rtti…
5ba69d5 
…-flag

PiperOrigin-RevId: 242038269
@gennadiycivil
Copy link
Contributor

Should be in now 5ba69d5

@ngie-eign ngie-eign deleted the clang-add-explicit-c++11-rtti-flag branch April 5, 2019 16:50
@ngie-eign
Copy link
Contributor Author

ngie-eign commented Apr 5, 2019 •
edited

The fact we failed to set cxx_no_rtti_flags with Clang seems like a bug? Do we not have a test that catches it?

We should have a test that catches this. I'll post something soon to add it to Travis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EricWF EricWF EricWF left review comments

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ngie-eign @gennadiycivil @EricWF @googlebot