|
|
|
Created:
17 years, 5 months ago by Balazs.Dan Modified:
11 years, 1 month ago Reviewers:
wan CC:
googletestframework_googlegroups.com Base URL:
http://googletest.googlecode.com/svn/trunk/ Visibility:
Public. |
Patch Set 1 #
Total messages: 4
|
Balazs.Dan
Hello guys, Please review this patch! Thx, Balazs
|
17 years, 5 months ago (2008年07月25日 23:40:04 UTC) #1 | |||||||||||||||||||||||||||||||||||||||||||||||||
Hello guys, Please review this patch! Thx, Balazs
http://codereview.appspot.com/2682/diff/1/5 File include/gtest/gtest_pred_impl.h (right): http://codereview.appspot.com/2682/diff/1/5#newcode79 Line 79: try { \ Let's get the ASSERT_THROW, etc, macros checked in first. Just a quick note: the code should compile when exceptions are disabled. As it is, this won't compile in this case.
http://codereview.appspot.com/2682/diff/1/5 File include/gtest/gtest_pred_impl.h (right): http://codereview.appspot.com/2682/diff/1/5#newcode100 Line 100: GTEST_CONCAT_TOKEN(gtest_label_assert_, __LINE__): \ This could break existing tests that use multiple assertions on the same line. Although people may not do that deliberately, it could happen as the result of macro expansion. Given this and the complexity of the change, I'd suggest to withdraw the patch. Now that EXPECT_THROW, etc are being added, this is less useful. I'm thinking it's not worth complicating the already complex implementation. If you had an unhandled exception in your test, it's easy to find who threw it under a debugger. I understand you have put a lot of effort in this. Unfortunately not every patch can eventually be checked in. Don't let this stop you from contributing more great work though! Thanks!
http://codereview.appspot.com/2682/diff/1/5 File include/gtest/gtest_pred_impl.h (right): http://codereview.appspot.com/2682/diff/1/5#newcode100 Line 100: GTEST_CONCAT_TOKEN(gtest_label_assert_, __LINE__): \ Well, you suggested on the forum to use a similar trick as you used in death test macros. Both death test macros and expect_throw macros are using the same trick thus also not possible to use more than 1 on the same line! This complexity is the result of message streaming. There are other very good and handy ways to display additional messages, but that would require even bigger modifications..