Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2168973002:
Remove obsolete gtest configurations (Closed)
DescriptionRemove obsolete gtest configurations
BUG=630299
Committed: https://crrev.com/5e583c4c8185bb70d268f59dd48bd7244913b3c4
Cr-Commit-Position: refs/heads/master@{#406943}
Patch Set 1 #
Total comments: 3
Dependent Patchsets: Messages
Total messages: 19 (10 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Remove obsolete gtest and gmock configurations BUG= ========== to ========== Remove obsolete gtest configurations BUG=630299 ==========
tzik@chromium.org changed reviewers: + dpranke@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dpranke@chromium.org changed reviewers: + danakj@chromium.org
I'm not close enough to the c++ standards to be sure this is safe. I'd normally guess that it is, but danakj@, can you take a look as well (I'd punt this to thakis@, but he's not available).
LGTM assuming gtest works with RTTI off in clang. Can you please confirm? https://codereview.chromium.org/2168973002/diff/1/build/secondary/testing/gte... File build/secondary/testing/gtest/BUILD.gn (left): https://codereview.chromium.org/2168973002/diff/1/build/secondary/testing/gte... build/secondary/testing/gtest/BUILD.gn:32: # versions older than 4.3.2, and assumes it's enabled. Our Mac I currently have gcc 4.8.4, so that's a long way from 4.3.2. Precise has 4.6.3. Natty has 4.5.2. Maverick has 4.4.4 and 4.5.1, and support for it ended in 2012. I think this should be safe to remove then, as according to the comment, gtest can figure out RTTI now. One question though, this comment doesn't mention clang. Can gtest tell we have RTTI disabled on clang? https://codereview.chromium.org/2168973002/diff/1/build/secondary/testing/gte... build/secondary/testing/gtest/BUILD.gn:48: "GTEST_USE_OWN_TR1_TUPLE=1", This looks ok to remove, we support variadic templates everywhere.
https://codereview.chromium.org/2168973002/diff/1/build/secondary/testing/gte... File build/secondary/testing/gtest/BUILD.gn (left): https://codereview.chromium.org/2168973002/diff/1/build/secondary/testing/gte... build/secondary/testing/gtest/BUILD.gn:32: # versions older than 4.3.2, and assumes it's enabled. Our Mac On 2016/07/21 18:54:58, danakj wrote: > I currently have gcc 4.8.4, so that's a long way from 4.3.2. Precise has 4.6.3. > Natty has 4.5.2. Maverick has 4.4.4 and 4.5.1, and support for it ended in 2012. > I think this should be safe to remove then, as according to the comment, gtest > can figure out RTTI now. > > One question though, this comment doesn't mention clang. Can gtest tell we have > RTTI disabled on clang? Yes. According to the comment around the feature detection in gtest, it can detect the availability of RTTI from clang 2.7, that was released in 2010. https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/internal/gte...
On 2016/07/21 19:06:24, tzik wrote: > https://codereview.chromium.org/2168973002/diff/1/build/secondary/testing/gte... > File build/secondary/testing/gtest/BUILD.gn (left): > > https://codereview.chromium.org/2168973002/diff/1/build/secondary/testing/gte... > build/secondary/testing/gtest/BUILD.gn:32: # versions older than 4.3.2, and > assumes it's enabled. Our Mac > On 2016/07/21 18:54:58, danakj wrote: > > I currently have gcc 4.8.4, so that's a long way from 4.3.2. Precise has > 4.6.3. > > Natty has 4.5.2. Maverick has 4.4.4 and 4.5.1, and support for it ended in > 2012. > > I think this should be safe to remove then, as according to the comment, gtest > > can figure out RTTI now. > > > > One question though, this comment doesn't mention clang. Can gtest tell we > have > > RTTI disabled on clang? > > Yes. According to the comment around the feature detection in gtest, it can > detect the availability of RTTI from clang 2.7, that was released in 2010. > https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/internal/gte... Great thanks. LGTM
lgtm
The CQ bit was checked by tzik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove obsolete gtest configurations BUG=630299 ========== to ========== Remove obsolete gtest configurations BUG=630299 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Remove obsolete gtest configurations BUG=630299 ========== to ========== Remove obsolete gtest configurations BUG=630299 Committed: https://crrev.com/5e583c4c8185bb70d268f59dd48bd7244913b3c4 Cr-Commit-Position: refs/heads/master@{#406943} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/5e583c4c8185bb70d268f59dd48bd7244913b3c4 Cr-Commit-Position: refs/heads/master@{#406943} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
