Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(68)

Issue 117080: Support for Symbian (WINSCW)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 1 month ago by mika.raento
Modified:
5 years, 10 months ago
Reviewers:
Zhanyong Wan
CC:
googlemock_googlegroups.com
Base URL:
http://googlemock.googlecode.com/svn/trunk/
Visibility:
Public.

Patch Set 1 #

Total comments: 42

Patch Set 2 : addressed review comments #

Patch Set 3 : synced with SVN, NativeArray still needs a cast #

Patch Set 4 : moved SafeMatcherCastImpl comments to correct places, wchar_t GMOCK_DECLARE_KIND #ifdef style fix #

Total comments: 11
Unified diffs Side-by-side diffs Delta from patch set Stats (+136 lines, -59 lines) Patch
M include/gmock/gmock-generated-matchers.h View 1 2 1 chunk +13 lines, -5 lines 0 comments Download
M include/gmock/gmock-matchers.h View 1 2 3 1 chunk +52 lines, -37 lines 0 comments Download
M include/gmock/internal/gmock-internal-utils.h View 1 2 3 2 chunks +28 lines, -1 line 11 comments Download
M test/gmock-actions_test.cc View 1 2 3 chunks +11 lines, -4 lines 0 comments Download
M test/gmock-internal-utils_test.cc View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M test/gmock-matchers_test.cc View 1 2 4 chunks +27 lines, -11 lines 0 comments Download
M test/gmock-printers_test.cc View 1 2 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 13
mika.raento
Dearest Googlemock I've uploaded a patch to add Symbian support to googlemock. This passes all ...
11 years, 1 month ago (2009-09-15 14:26:40 UTC) #1
Zhanyong Wan
Thanks for another high-quality patch, Mika! http://codereview.appspot.com/117080/diff/1/6 File include/gmock/gmock-generated-matchers.h (right): http://codereview.appspot.com/117080/diff/1/6#newcode312 Line 312: Matcher<Container> m; ...
11 years, 1 month ago (2009-09-15 21:32:50 UTC) #2
mika.raento
http://codereview.appspot.com/117080/diff/1/6 File include/gmock/gmock-generated-matchers.h (right): http://codereview.appspot.com/117080/diff/1/6#newcode312 Line 312: Matcher<Container> m; On 2009/09/15 21:32:50, Zhanyong Wan wrote: ...
11 years, 1 month ago (2009-09-16 14:21:29 UTC) #3
Zhanyong Wan
http://codereview.appspot.com/117080/diff/1/8 File include/gmock/internal/gmock-internal-utils.h (right): http://codereview.appspot.com/117080/diff/1/8#newcode261 Line 261: #if defined(__SYMBIAN32__) && defined(_STLP_NO_WCHAR_T) On 2009/09/16 14:21:29, mika.raento ...
11 years, 1 month ago (2009-09-16 17:05:55 UTC) #4
mika.raento
Synced to svn and addressed review comments. NativeArray still requires a cast - the compiler ...
11 years, 1 month ago (2009-09-19 07:44:25 UTC) #5
Zhanyong Wan
http://codereview.appspot.com/117080/diff/3008/3015 File include/gmock/internal/gmock-internal-utils.h (right): http://codereview.appspot.com/117080/diff/3008/3015#newcode702 Line 702: return type(const_cast<Element*>(&array[0]), N, kReference); I think the compiler ...
11 years, 1 month ago (2009-09-21 19:34:05 UTC) #6
mika.raento
http://codereview.appspot.com/117080/diff/3008/3015 File include/gmock/internal/gmock-internal-utils.h (right): http://codereview.appspot.com/117080/diff/3008/3015#newcode702 Line 702: return type(const_cast<Element*>(&array[0]), N, kReference); On 2009/09/21 19:34:05, Zhanyong ...
11 years, 1 month ago (2009-09-22 12:13:34 UTC) #7
Zhanyong Wan
http://codereview.appspot.com/117080/diff/3008/3015 File include/gmock/internal/gmock-internal-utils.h (right): http://codereview.appspot.com/117080/diff/3008/3015#newcode702 Line 702: return type(const_cast<Element*>(&array[0]), N, kReference); On 2009/09/22 12:13:34, mika.raento ...
11 years, 1 month ago (2009-09-22 22:16:24 UTC) #8
Zhanyong Wan
http://codereview.appspot.com/117080/diff/3008/3015 File include/gmock/internal/gmock-internal-utils.h (right): http://codereview.appspot.com/117080/diff/3008/3015#newcode702 Line 702: return type(const_cast<Element*>(&array[0]), N, kReference); On 2009/09/22 22:16:24, Zhanyong ...
11 years, 1 month ago (2009-09-22 22:20:29 UTC) #9
Zhanyong Wan
http://codereview.appspot.com/117080/diff/3008/3015 File include/gmock/internal/gmock-internal-utils.h (right): http://codereview.appspot.com/117080/diff/3008/3015#newcode702 Line 702: return type(const_cast<Element*>(&array[0]), N, kReference); I took a closer ...
11 years, 1 month ago (2009-09-23 00:04:33 UTC) #10
Zhanyong Wan
http://codereview.appspot.com/117080/diff/3008/3015 File include/gmock/internal/gmock-internal-utils.h (right): http://codereview.appspot.com/117080/diff/3008/3015#newcode702 Line 702: return type(const_cast<Element*>(&array[0]), N, kReference); This appears to work ...
11 years, 1 month ago (2009-09-23 06:53:47 UTC) #11
mika.raento
Thanks a lot for all your help and patience wan. http://codereview.appspot.com/117080/diff/3008/3015 File include/gmock/internal/gmock-internal-utils.h (right): http://codereview.appspot.com/117080/diff/3008/3015#newcode702 ...
11 years, 1 month ago (2009-09-23 07:21:56 UTC) #12
Zhanyong Wan
11 years, 1 month ago (2009-09-23 17:29:13 UTC) #13
http://codereview.appspot.com/117080/diff/3008/3015
File include/gmock/internal/gmock-internal-utils.h (right):

http://codereview.appspot.com/117080/diff/3008/3015#newcode702
Line 702: return type(const_cast<Element*>(&array[0]), N, kReference);
On 2009/09/23 07:21:56, mika.raento wrote:

> It does work for the case that causes the original problem but crashes the
> compiler in an unrelated case :-( :
> 
> ..\third_party\googlemock\include\gmock\gmock-matchers.h:1742: internal
compiler
> error (please report at <http://www.forum.nokia.com>)
> scons: ***
> [C:\software\karhea\third_party\googlemock\test\gmock-matchers_test.o]
> 
> gmock-matchers.h:1742 is
>   explicit ContainerEqMatcher(const Container& rhs) : rhs_(View::Copy(rhs)) {
> 
> and if I comment out some of the ContainerEq tests in gmock-matchers_test.cc I
> get a silent crash instead.

ICE!  Silent crash!!  Holy crap!!!

OK, I throw up my hands.

http://codereview.appspot.com/117080/diff/3008/3015#newcode702
Line 702: return type(const_cast<Element*>(&array[0]), N, kReference);
On 2009/09/23 07:21:56, mika.raento wrote:

> Now the reason I've been proposing const_cast<> (and maybe I've misunderstood,
> please correct) is that the testing::StaticAssertTypeEq<Element,
RawElement>();
> should force the _user_ of this code to be correct and the #else branch of the
> #if keeps the original code for other platformss which forces GMock to be
> correct. Is there something I'm missing?

Yes, this particular use of const_cast is safe.  My concern is that the fact is
not obvious.  A reader needs to spend more time here to figure out why it's
safe.  Ideally, we should write it in a way that's obviously correct.  Sadly, it
doesn't seem easy, and I'm willing to concede to the Symbian compiler god now.

The reason I want to get to the bottom of this is that I'm not sure the
const_cast really fixes the root problem.  It apparently works for char*[4], but
what about char**[4]?  Or char*const*[5][6]?  Are we sure we only need
const_cast at the top level?

I don't have the time to fully investigate this, and don't want to block you. 
Therefore let's just shove it in now and deal with the problem if we find cases
where it doesn't work later.

I'll check in the patch once it passes my tests and another Googler's review. 
Thanks.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b