Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2227073002:
cctest: put some types into anonymous namespace. (Closed)
Descriptioncctest: put some types into anonymous namespace.
This is to prevent a collision with Expectations class defined in test-field-type-tracking.cc, which happens in Clang under certain conditions (official build + Precise + unknown).
While the original intent was to just workaround the Clang bug, putting types into anonymous namespace seems reasonable anyway (thank you to Benedikt Meurer for the suggestion!)
BUG=630335
Committed: https://crrev.com/a9b5eb0c5958c41691b779536c614866c9c901f0
Cr-Commit-Position: refs/heads/master@{#38552}
Patch Set 1 #
Total comments: 2
Patch Set 2 : cctest: put some types into anonymous namespace. #Messages
Total messages: 25 (15 generated)
krasin@google.com changed reviewers: + thakis@chromium.org
thakis@chromium.org changed reviewers: + machenbach@chromium.org
I'm not a v8 reviewer. machenbach, can you take a look or add a good reviewre?
machenbach@chromium.org changed reviewers: + bmeurer@chromium.org, mstarzinger@chromium.org - thakis@chromium.org
Adding V8 reviewers. PTAL
The CQ bit was checked by machenbach@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2227073002/diff/1/test/cctest/test-decls.cc File test/cctest/test-decls.cc (right): https://codereview.chromium.org/2227073002/diff/1/test/cctest/test-decls.cc#n... test/cctest/test-decls.cc:39: // types: here and in test-field-type-tracking.cc, which makes the How about just using an anonymous namespace?
On 2016/08/10 08:09:33, Benedikt Meurer wrote: > https://codereview.chromium.org/2227073002/diff/1/test/cctest/test-decls.cc > File test/cctest/test-decls.cc (right): > > https://codereview.chromium.org/2227073002/diff/1/test/cctest/test-decls.cc#n... > test/cctest/test-decls.cc:39: // types: here and in test-field-type-tracking.cc, > which makes the > How about just using an anonymous namespace? Could you add that change so that it's ready when MTV day starts again?
The CQ bit was checked by krasin@google.com 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...
Please, take another look. While it's not guaranteed to work, it is much nicer and does not feel like a temporary workaround anymore. https://codereview.chromium.org/2227073002/diff/1/test/cctest/test-decls.cc File test/cctest/test-decls.cc (right): https://codereview.chromium.org/2227073002/diff/1/test/cctest/test-decls.cc#n... test/cctest/test-decls.cc:39: // types: here and in test-field-type-tracking.cc, which makes the On 2016/08/10 08:09:33, Benedikt Meurer wrote: > How about just using an anonymous namespace? Done.
Description was changed from
==========
Workaround a Clang bug to fix flakiness in the official build.
Clang (under certain conditions, like LTO + Precise + unknown) confuses
two Expectations types (an enum and a class) and fails with an internal
error. Temporarily renaming one of the types to get rid of the name
collision before the underlying bug is fixed.
BUG=630335
==========
to
==========
cctest: put some types into anonymous namespace.
This is to prevent a collision with Expectations class defined in
test-field-type-tracking.cc, which happens in Clang under certain conditions
(official build + Precise + unknown).
While the original intent was to just workaround the Clang bug, putting types
into anonymous namespace seems reasonable anyway (thank you to Benedikt Meurer
for the suggestion!)
BUG=630335
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, LGTM.
The CQ bit was checked by krasin@google.com
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
==========
cctest: put some types into anonymous namespace.
This is to prevent a collision with Expectations class defined in
test-field-type-tracking.cc, which happens in Clang under certain conditions
(official build + Precise + unknown).
While the original intent was to just workaround the Clang bug, putting types
into anonymous namespace seems reasonable anyway (thank you to Benedikt Meurer
for the suggestion!)
BUG=630335
==========
to
==========
cctest: put some types into anonymous namespace.
This is to prevent a collision with Expectations class defined in
test-field-type-tracking.cc, which happens in Clang under certain conditions
(official build + Precise + unknown).
While the original intent was to just workaround the Clang bug, putting types
into anonymous namespace seems reasonable anyway (thank you to Benedikt Meurer
for the suggestion!)
BUG=630335
==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from
==========
cctest: put some types into anonymous namespace.
This is to prevent a collision with Expectations class defined in
test-field-type-tracking.cc, which happens in Clang under certain conditions
(official build + Precise + unknown).
While the original intent was to just workaround the Clang bug, putting types
into anonymous namespace seems reasonable anyway (thank you to Benedikt Meurer
for the suggestion!)
BUG=630335
==========
to
==========
cctest: put some types into anonymous namespace.
This is to prevent a collision with Expectations class defined in
test-field-type-tracking.cc, which happens in Clang under certain conditions
(official build + Precise + unknown).
While the original intent was to just workaround the Clang bug, putting types
into anonymous namespace seems reasonable anyway (thank you to Benedikt Meurer
for the suggestion!)
BUG=630335
Committed: https://crrev.com/a9b5eb0c5958c41691b779536c614866c9c901f0
Cr-Commit-Position: refs/heads/master@{#38552}
==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a9b5eb0c5958c41691b779536c614866c9c901f0 Cr-Commit-Position: refs/heads/master@{#38552} |
