Support for Symbian (WINSCW)

[mika.raento, 2009-09-15 14:26:40]

Dearest Googlemock

I've uploaded a patch to add Symbian support to googlemock. 

This passes all the 'normal' tests - tests that don't require a specific test
runner for verification as hooking one up to the Symbian emulator is a major
pain.

The patch does just disable a couple of tests and NativeArray doesn't work fully
- I hope you can give feedback on whether these are things we can live with.

Doing this also uncovered a nasty bug in the winscw (emulator) compiler: under
the right conditions the destructor is not called for objects copied by value
into a local array. I was unable to create a more minimal test case but will
look into it deeper.

It would be extremely neat if somebody in the mobile team at G could later set
up a continuous build for this...

Cheers,

   Mika

[Zhanyong Wan, 2009-09-15 21:32:50]

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;
My understanding is that the same change needs to be done for other arities as
well.

Looks like it's time to opensource the Pump script finally. I'll take care of
updating the .pump file when the review is done this time.

http://codereview.appspot.com/117080/diff/1/7
File include/gmock/gmock-matchers.h (right):

http://codereview.appspot.com/117080/diff/1/7#newcode48
Line 48: #include <e32svr.h>
What is this for?  Also, shouldn't it be #if on Symbian only?

http://codereview.appspot.com/117080/diff/1/7#newcode330
Line 330: template<typename T>
Nit: a space after "template" to be consistent.

http://codereview.appspot.com/117080/diff/1/7#newcode333
Line 333: template<typename M>
The same.

http://codereview.appspot.com/117080/diff/1/7#newcode337
Line 337: template <typename U>
Nit: a blank line before this?

http://codereview.appspot.com/117080/diff/1/7#newcode549
Line 549: source_matcher_.ExplainMatchResultTo(U(x), os);
This changes the intended behavior.  Why's it necessary?

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)
Merge this with the #elif below?

http://codereview.appspot.com/117080/diff/1/8#newcode709
Line 709: return internal::NativeArray<RawElement>::NativeArray(arr_tuple,
Does it compile to replace

  internal::NativeArray<RawElement>

with

  type

?

http://codereview.appspot.com/117080/diff/1/8#newcode709
Line 709: return internal::NativeArray<RawElement>::NativeArray(arr_tuple,
Does it compile to write

  internal::NativeArray<RawElement>::NativeArray<N>(array, kReference)

?

http://codereview.appspot.com/117080/diff/1/8#newcode711
Line 711: #else  // !GTEST_OS_SYMBIAN
Actually, I have an idea on simplifying the definition of NativeArray, which may
make the hack unnecessary, or at least simpler.  Let me try that first.

http://codereview.appspot.com/117080/diff/1/2
File test/gmock-actions_test.cc (right):

http://codereview.appspot.com/117080/diff/1/2#newcode104
Line 104: #endif  // !GTEST_OS_WINDOWS
This comment doesn't match the #if comment.  I'll just remove it.

http://codereview.appspot.com/117080/diff/1/2#newcode130
Line 130: #endif  // !GTEST_OS_WINDOWS
The same.

http://codereview.appspot.com/117080/diff/1/5
File test/gmock-internal-utils_test.cc (right):

http://codereview.appspot.com/117080/diff/1/5#newcode218
Line 218: EXPECT_EQ(NULL, GetRawPointer(p));
I'll just replace this with the Symbian version.  One version that works for
both cases is enough.

http://codereview.appspot.com/117080/diff/1/5#newcode857
Line 857: NativeArray<int> na(::std::tr1::tuple<int*, size_t>(*a, 2), kCopy);
Please wait for my simplification to NativeArray to be checked in first.

http://codereview.appspot.com/117080/diff/1/5#newcode887
Line 887: NativeArray<int> na(::std::tr1::tuple<const int*, size_t>(a, 3),
kCopy);
The same.

http://codereview.appspot.com/117080/diff/1/3
File test/gmock-matchers_test.cc (right):

http://codereview.appspot.com/117080/diff/1/3#newcode879
Line 879: // std::make_pair(1, "foo") gives
Can this comment fit on 2 lines?

http://codereview.appspot.com/117080/diff/1/3#newcode881
Line 881: // on Nokia's Symbian compiler
End this with a period.

http://codereview.appspot.com/117080/diff/1/3#newcode882
Line 882: container.insert(std::make_pair(1, string("foo")));
Use std::string instead of string.

http://codereview.appspot.com/117080/diff/1/3#newcode1829
Line 1829: // ASSERT_THAT("hello", starts_with_he) fails to compile with Nokia's
Does it still fail with your change to SafeMatcherCast?

http://codereview.appspot.com/117080/diff/1/3#newcode1842
Line 1842: ASSERT_THAT("hello", starts_with_he);
Does this work on Symbian?

  const char* s = "hello";
  ASSERT_THAT(s, starts_with_he);

http://codereview.appspot.com/117080/diff/1/4
File test/gmock-printers_test.cc (right):

http://codereview.appspot.com/117080/diff/1/4#newcode296
Line 296: // among other things formatting 9.5 as "9.".
So you are testing the OpenC implementation instead of gmock.  Why?

[mika.raento, 2009-09-16 14:21:29]

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:

Hm, not quite: the bug was only triggered by the 1-element version.

I also whittled down a testcase and it doesn't happen with 2 elements.

Sent a bug report to Symbian,
http://developer.symbian.org/bugs/show_bug.cgi?id=368

http://codereview.appspot.com/117080/diff/1/7
File include/gmock/gmock-matchers.h (right):

http://codereview.appspot.com/117080/diff/1/7#newcode48
Line 48: #include <e32svr.h>
On 2009/09/15 21:32:50, Zhanyong Wan wrote:

Bah - it was needed for debug logging, forgot to remove. Removed.

http://codereview.appspot.com/117080/diff/1/7#newcode330
Line 330: template<typename T>
On 2009/09/15 21:32:50, Zhanyong Wan wrote:

Done.

http://codereview.appspot.com/117080/diff/1/7#newcode333
Line 333: template<typename M>
On 2009/09/15 21:32:50, Zhanyong Wan wrote:

Done.

http://codereview.appspot.com/117080/diff/1/7#newcode337
Line 337: template <typename U>
On 2009/09/15 21:32:50, Zhanyong Wan wrote:

Done.

http://codereview.appspot.com/117080/diff/1/7#newcode549
Line 549: source_matcher_.ExplainMatchResultTo(U(x), os);
On 2009/09/15 21:32:50, Zhanyong Wan wrote:

Reverted.

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/15 21:32:50, Zhanyong Wan wrote:

Hm. Felt less readable:
#if !(defined(__SYMBIAN32__) && defined(_STLP_NO_WCHAR_T)) &&
(!defined(_MSC_VER) || defined(_NATIVE_WCHAR_T_DEFINED))
?

http://codereview.appspot.com/117080/diff/1/8#newcode709
Line 709: return internal::NativeArray<RawElement>::NativeArray(arr_tuple,
On 2009/09/15 21:32:50, Zhanyong Wan wrote:

No. Compiler says that 'N' is not declared (so it's well confused :-( ).

http://codereview.appspot.com/117080/diff/1/8#newcode709
Line 709: return internal::NativeArray<RawElement>::NativeArray(arr_tuple,
On 2009/09/15 21:32:50, Zhanyong Wan wrote:

Yes, done.

http://codereview.appspot.com/117080/diff/1/8#newcode711
Line 711: #else  // !GTEST_OS_SYMBIAN
On 2009/09/15 21:32:50, Zhanyong Wan wrote:

ok.

http://codereview.appspot.com/117080/diff/1/2
File test/gmock-actions_test.cc (right):

http://codereview.appspot.com/117080/diff/1/2#newcode104
Line 104: #endif  // !GTEST_OS_WINDOWS
On 2009/09/15 21:32:50, Zhanyong Wan wrote:

Done.

http://codereview.appspot.com/117080/diff/1/2#newcode130
Line 130: #endif  // !GTEST_OS_WINDOWS
On 2009/09/15 21:32:50, Zhanyong Wan wrote:

Done.

http://codereview.appspot.com/117080/diff/1/5
File test/gmock-internal-utils_test.cc (right):

http://codereview.appspot.com/117080/diff/1/5#newcode218
Line 218: EXPECT_EQ(NULL, GetRawPointer(p));
On 2009/09/15 21:32:50, Zhanyong Wan wrote:

Done.

http://codereview.appspot.com/117080/diff/1/3
File test/gmock-matchers_test.cc (right):

http://codereview.appspot.com/117080/diff/1/3#newcode879
Line 879: // std::make_pair(1, "foo") gives
On 2009/09/15 21:32:50, Zhanyong Wan wrote:

Done.

http://codereview.appspot.com/117080/diff/1/3#newcode881
Line 881: // on Nokia's Symbian compiler
On 2009/09/15 21:32:50, Zhanyong Wan wrote:

Done.

http://codereview.appspot.com/117080/diff/1/3#newcode882
Line 882: container.insert(std::make_pair(1, string("foo")));
On 2009/09/15 21:32:50, Zhanyong Wan wrote:

Done.

http://codereview.appspot.com/117080/diff/1/3#newcode1829
Line 1829: // ASSERT_THAT("hello", starts_with_he) fails to compile with Nokia's
On 2009/09/15 21:32:50, Zhanyong Wan wrote:

Yeah.

http://codereview.appspot.com/117080/diff/1/3#newcode1842
Line 1842: ASSERT_THAT("hello", starts_with_he);
On 2009/09/15 21:32:50, Zhanyong Wan wrote:


No.

http://codereview.appspot.com/117080/diff/1/4
File test/gmock-printers_test.cc (right):

http://codereview.appspot.com/117080/diff/1/4#newcode296
Line 296: // among other things formatting 9.5 as "9.".
On 2009/09/15 21:32:50, Zhanyong Wan wrote:I got 

I got failures in print tests that had 9.5 as part of a tuple. It was easier to
narrow down the source of the problem this way (less moving parts) so I thought
I'd leave it in in case anybody else needs to diagnose the same problem.

I wouldn't mind removing it.

[Zhanyong Wan, 2009-09-16 17:05:55]

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 wrote:
> On 2009/09/15 21:32:50, Zhanyong Wan wrote:
> 
> Hm. Felt less readable:
> #if !(defined(__SYMBIAN32__) && defined(_STLP_NO_WCHAR_T)) &&
> (!defined(_MSC_VER) || defined(_NATIVE_WCHAR_T_DEFINED))
> ?

I think the following is more readable - it is a direct translation of the
comment above.

(Any reason we cannot use GTEST_OS_SYMBIAN?)

#if (GTEST_OS_SYMBIAN && defined(_STLP_NO_WCHAR_T)) || \
    (defined(_MSC_VER) && !defined(_NATIVE_WCHAR_T_DEFINED))
// wchar_t is a typedef - do nothing.
#else
GMOCK_DECLARE_KIND_(wchar_t, kInteger);
#endif

[mika.raento, 2009-09-19 07:44:25]

Synced to svn and addressed review comments. NativeArray still requires a cast -
the compiler confuses itself with the template Iterator type.

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 17:05:56, Zhanyong Wan wrote:
> On 2009/09/16 14:21:29, mika.raento wrote:
> > On 2009/09/15 21:32:50, Zhanyong Wan wrote:
> > 
> > Hm. Felt less readable:
> > #if !(defined(__SYMBIAN32__) && defined(_STLP_NO_WCHAR_T)) &&
> > (!defined(_MSC_VER) || defined(_NATIVE_WCHAR_T_DEFINED))
> > ?
> 
> I think the following is more readable - it is a direct translation of the
> comment above.
> 
> (Any reason we cannot use GTEST_OS_SYMBIAN?)
> 
> #if (GTEST_OS_SYMBIAN && defined(_STLP_NO_WCHAR_T)) || \
>     (defined(_MSC_VER) && !defined(_NATIVE_WCHAR_T_DEFINED))
> // wchar_t is a typedef - do nothing.
> #else
> GMOCK_DECLARE_KIND_(wchar_t, kInteger);
> #endif

Done.

[Zhanyong Wan, 2009-09-21 19:34:05]

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 is parsing &array[0] as (&array)[0], which doesn't have the
right type.

Does type(array, N, kReference) compile?  If not, how about

  type(array + 0, N, kReference)

or

  type(&(array[0]), N, kReference)

?

If one of the above works, please use it for both Symbian and non-Symbian.

http://codereview.appspot.com/117080/diff/3008/3015#newcode709
Line 709: return type(const_cast<Element*>(&array[0]), N, kCopy);
The same.

[mika.raento, 2009-09-22 12:13:34]

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 Wan wrote:
> I think the compiler is parsing &array[0] as (&array)[0], which doesn't have
the
> right type.
> 
> Does type(array, N, kReference) compile?  If not, how about
> 
>   type(array + 0, N, kReference)
> 
> or
> 
>   type(&(array[0]), N, kReference)
> 
> ?
> 
> If one of the above works, please use it for both Symbian and non-Symbian.

I did start out with type(array, N, kReference). None of these alternatives
work.

The problem only occurs when instantiating with an array of char arrays. The
error with &(array[0]) is

function call '[testing::internal::NativeArray<char *>].NativeArray({lval} const
char **, long, testing::internal::RelationToSource)'

does not match
'testing::internal::NativeArray<char *>::NativeArray(char *const *, unsigned
int, testing::internal::RelationToSource)'.

[Zhanyong Wan, 2009-09-22 22:16:24]

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 wrote:
> On 2009/09/21 19:34:05, Zhanyong Wan wrote:
> > I think the compiler is parsing &array[0] as (&array)[0], which doesn't have
> the
> > right type.
> > 
> > Does type(array, N, kReference) compile?  If not, how about
> > 
> >   type(array + 0, N, kReference)
> > 
> > or
> > 
> >   type(&(array[0]), N, kReference)
> > 
> > ?
> > 
> > If one of the above works, please use it for both Symbian and non-Symbian.
> 
> I did start out with type(array, N, kReference). None of these alternatives
> work.
> 
> The problem only occurs when instantiating with an array of char arrays. The
> error with &(array[0]) is
> 
> function call '[testing::internal::NativeArray<char *>].NativeArray({lval}
const
> char **, long, testing::internal::RelationToSource)'
> 
> does not match
> 'testing::internal::NativeArray<char *>::NativeArray(char *const *, unsigned
> int, testing::internal::RelationToSource)'.

const_cast bypasses the type system and can easily hide bugs.  I'd like to get
to the bottom of this.  Could you show me the exact test code that triggers this
error?  Thanks.

[Zhanyong Wan, 2009-09-22 22:20:29]

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 Wan wrote:

> const_cast bypasses the type system and can easily hide bugs.  I'd like to get
> to the bottom of this.  Could you show me the exact test code that triggers
this
> error?  Thanks.

Is it this one?

TEST(NativeArrayTest, WorksForTwoDimensionalArray) {
  const char a[2][3] = { "hi", "lo" };
  NativeArray<char[3]> na(a, 2, kReference);
  ...
}

[Zhanyong Wan, 2009-09-23 00:04:33]

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 look at the compiler error you supplied in the
comment:

689 // The Nokia Symbian compiler confuses itself in template instantiation
690 // for this call without the cast to Element*:
691 // function call '[testing::internal::NativeArray<char *>].NativeArray(
692 // {lval} const char *[4], long, testing::internal::RelationToSource)'
693 // does not match
694 // 'testing::internal::NativeArray<char *>::NativeArray(
695 // char *const *, unsigned int, testing::internal::RelationToSource)'
696 // (instantiating: 'testing::internal::ContainsMatcherImpl
697 // <const char * (&)[4]>::Matches(const char * (&)[4]) const')
698 // (instantiating: 'testing::internal::StlContainerView<char *[4]>::
699 // ConstReference(const char * (&)[4])')

Line 698 says the compiler is instantiating the type

  StlContainerView<char *[4]>

which corresponds to

  Element = char *
  N = 4

Our definition of the ConstReference() method is:

  ... ConstReference(const Element (&array)[N]) { .. }

i.e.

  ... ConstReference(char * const (&array)[4]) { .. }

However, according to line 699, the compiler thinks its type is:

  ... ConstReference(const char * (&array)[4]);

Looks like it naively think that 'const Element' is 'const char*'.

I'll see if I can find a way to work around it.  If you have time,
could you try if replacing "const Element" with "Element const" in the
signature of ConstReference() helps?

[Zhanyong Wan, 2009-09-23 06:53:47]

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 on both Symbian and non-Symbian:

  // ConstReference() is meant to take a const Element (&)[N], but
  // Nokia's Symbian compiler chokes on it when
  // StlContainerView<char*[4]> is instantiated, as it wrongfully
  // thinks that const Element is const char* when Element is char*.
  // Making ConstReference() take an array of unrelated type bypasses
  // the bug.
  template <typename T>
  static const_reference ConstReference(T (&array)[N]) {
    // Ensures that Element is not a const type.
    testing::StaticAssertTypeEq<Element, RawElement>();
    // Ensures that array has the right element type.
    testing::StaticAssertTypeEq<RawElement, GMOCK_REMOVE_CONST_(T)>();
    return type(array, N, kReference);
  }

  template <typename T>
  static type Copy(T (&array)[N]) {
    // Ensures that array has the right element type.
    testing::StaticAssertTypeEq<RawElement, GMOCK_REMOVE_CONST_(T)>();
    return type(array, N, kCopy);
  }

I think it's a cleaner fix as it avoids the dangerous const_cast.

[mika.raento, 2009-09-23 07:21:56]

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
Line 702: return type(const_cast<Element*>(&array[0]), N, kReference);
On 2009/09/23 06:53:48, Zhanyong Wan wrote:
> This appears to work on both Symbian and non-Symbian:
> 
>   // ConstReference() is meant to take a const Element (&)[N], but
>   // Nokia's Symbian compiler chokes on it when
>   // StlContainerView<char*[4]> is instantiated, as it wrongfully
>   // thinks that const Element is const char* when Element is char*.
>   // Making ConstReference() take an array of unrelated type bypasses
>   // the bug.
>   template <typename T>
>   static const_reference ConstReference(T (&array)[N]) {
>     // Ensures that Element is not a const type.
>     testing::StaticAssertTypeEq<Element, RawElement>();
>     // Ensures that array has the right element type.
>     testing::StaticAssertTypeEq<RawElement, GMOCK_REMOVE_CONST_(T)>();
>     return type(array, N, kReference);
>   }
> 
>   template <typename T>
>   static type Copy(T (&array)[N]) {
>     // Ensures that array has the right element type.
>     testing::StaticAssertTypeEq<RawElement, GMOCK_REMOVE_CONST_(T)>();
>     return type(array, N, kCopy);
>   }
> 
> I think it's a cleaner fix as it avoids the dangerous const_cast.

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.

http://codereview.appspot.com/117080/diff/3008/3015#newcode702
Line 702: return type(const_cast<Element*>(&array[0]), N, kReference);
On 2009/09/23 00:04:33, Zhanyong Wan wrote:
> I took a closer look at the compiler error you supplied in the
> comment:
> 
> 689 // The Nokia Symbian compiler confuses itself in template instantiation
> 690 // for this call without the cast to Element*:
> 691 // function call '[testing::internal::NativeArray<char *>].NativeArray(
> 692 // {lval} const char *[4], long, testing::internal::RelationToSource)'
> 693 // does not match
> 694 // 'testing::internal::NativeArray<char *>::NativeArray(
> 695 // char *const *, unsigned int, testing::internal::RelationToSource)'
> 696 // (instantiating: 'testing::internal::ContainsMatcherImpl
> 697 // <const char * (&)[4]>::Matches(const char * (&)[4]) const')
> 698 // (instantiating: 'testing::internal::StlContainerView<char *[4]>::
> 699 // ConstReference(const char * (&)[4])')
> 
> Line 698 says the compiler is instantiating the type
> 
>   StlContainerView<char *[4]>
> 
> which corresponds to
> 
>   Element = char *
>   N = 4
> 
> Our definition of the ConstReference() method is:
> 
>   ... ConstReference(const Element (&array)[N]) { .. }
> 
> i.e.
> 
>   ... ConstReference(char * const (&array)[4]) { .. }
> 
> However, according to line 699, the compiler thinks its type is:
> 
>   ... ConstReference(const char * (&array)[4]);
> 
> Looks like it naively think that 'const Element' is 'const char*'.
> 
> I'll see if I can find a way to work around it.  If you have time,
> could you try if replacing "const Element" with "Element const" in the
> signature of ConstReference() helps?
> 

const Element -> Element const doesn't help, the error is the same.

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?

[Zhanyong Wan, 2009-09-23 17:29: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.