Add atomicops from V8.

Add atomicops from V8.

This is needed to provide a better implementation of GoogleOnceInit() on which
depend the static initializers removal.

BUG=351

[kenton, 2012-02-17 00:05:53]

Thanks for doing this.  It will be nice to have these atomic ops available to
the protobuf library.

I think we have some users who use compilers other than GCC and MSVC.  Any idea
what to do about them?  Just abandoning them seems harsh but supporting them
seems like it would be hard too.

One thing we could do is allow them to define a macro
GOOGLE_PROTOBUF_NO_THREADSAFETY which people could #define if they don't care
about thread-safety, and then we'd just use naive thread-hostile implementations
of all the atomic ops.  We have actually received requests for an option like
this from people working on embedded platforms that don't have pthreads, since
the current code refuses to compile unless either Win32 or pthreads is
available.

It's too bad that the MSVC implementations of these operations have to be
out-of-line, but we definitely can't #include windows.h.  I hate windows.h so
much.

https://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/at...
File src/google/protobuf/stubs/atomicops.h (right):

https://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/at...
src/google/protobuf/stubs/atomicops.h:54: namespace protobuf {
Please put in google::protobuf::internal, since we don't want clients calling
these.

https://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/co...
File src/google/protobuf/stubs/common.h (right):

https://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/co...
src/google/protobuf/stubs/common.h:165: typedef int8   int8_t;
I'm concerned that these typedefs could cause confusion.  First, in theory they
could differ from the ones defined in stdint.h, e.g. int vs. long on IA-32 are
different types of the same size.  Second, you've defined them in the protobuf
namespace, which would lead to inconsistency between MSVC and GCC builds of
protobufs, possibly making maintenance more difficult.

Could you just change the atomicops code to not use the _t suffix?

https://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/gl...
File src/google/protobuf/stubs/globals.h (right):

https://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/gl...
src/google/protobuf/stubs/globals.h:1: // Copyright 2011 the GOOGLE_PROTOBUF
project authors. All rights reserved.
"GOOGLE_PROTOBUF": find/replace error?

https://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/gl...
src/google/protobuf/stubs/globals.h:1: // Copyright 2011 the GOOGLE_PROTOBUF
project authors. All rights reserved.
Rename this file to platform_macros.h.

https://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/gl...
src/google/protobuf/stubs/globals.h:120: // Define our own macros for writing
64-bit constants.  This is less fragile
Please delete any macros that aren't used by atomicops.  These ones look
particularly suspicious.

https://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/gl...
src/google/protobuf/stubs/globals.h:156: #define USING_BSD_ABI
Missing GOOGLE_PROTOBUF_.

https://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/gl...
src/google/protobuf/stubs/globals.h:159: } } // namespace google::protobuf
Please match style of other protobuf source files, here and in other files.

[philippe, 2012-02-17 17:08:36]

Hi Kenton,

Thanks for reviewing my CL.

I added the flag you mentioned and made the compilation of atomicops.h and
atomicops_internals_x86_msvc.cc conditional. For now this change does not affect
anyone as atomicops is not used (compiled) yet.

However, as you said, we may have to support the non-MSVC/GCC users. The flag
does not help all of them, as these users are not necessarily the ones who don't
care about thread safety. The question is what compilers do they use/how could
we know that?

I will also provide a naive implementation of GoogleOnceInit() in a next change
covering the case where the flag is defined.

About this set of changes (removing static initializers), I also created some
internal CLs (Chrome for Android repository) to let us submit the change soon
(without addressing all the multi-platform issues).

Cheers,
Philippe.

http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/ato...
File src/google/protobuf/stubs/atomicops.h (right):

http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/ato...
src/google/protobuf/stubs/atomicops.h:54: namespace protobuf {
On 2012/02/17 00:05:54, kenton wrote:
> Please put in google::protobuf::internal, since we don't want clients calling
> these.

Done.

http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/com...
File src/google/protobuf/stubs/common.h (right):

http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/com...
src/google/protobuf/stubs/common.h:165: typedef int8   int8_t;
On 2012/02/17 00:05:54, kenton wrote:
> I'm concerned that these typedefs could cause confusion.  First, in theory
they
> could differ from the ones defined in stdint.h, e.g. int vs. long on IA-32 are
> different types of the same size.  Second, you've defined them in the protobuf
> namespace, which would lead to inconsistency between MSVC and GCC builds of
> protobufs, possibly making maintenance more difficult.
> 
> Could you just change the atomicops code to not use the _t suffix?

Done.

http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/glo...
File src/google/protobuf/stubs/globals.h (right):

http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/glo...
src/google/protobuf/stubs/globals.h:1: // Copyright 2011 the GOOGLE_PROTOBUF
project authors. All rights reserved.
On 2012/02/17 00:05:54, kenton wrote:
> "GOOGLE_PROTOBUF": find/replace error?

Indeed :)

http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/glo...
src/google/protobuf/stubs/globals.h:120: // Define our own macros for writing
64-bit constants.  This is less fragile
On 2012/02/17 00:05:54, kenton wrote:
> Please delete any macros that aren't used by atomicops.  These ones look
> particularly suspicious.

Done.

http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/glo...
src/google/protobuf/stubs/globals.h:156: #define USING_BSD_ABI
On 2012/02/17 00:05:54, kenton wrote:
> Missing GOOGLE_PROTOBUF_.

Done.

http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/glo...
src/google/protobuf/stubs/globals.h:159: } } // namespace google::protobuf
On 2012/02/17 00:05:54, kenton wrote:
> Please match style of other protobuf source files, here and in other files.

I removed the namespace since we renamed the file to platform_macros.h which
should restrict its content to macros.

[kenton, 2012-02-17 20:23:57]

Regarding other platforms:  My thought was that we can say: "If your
compiler is not supported, *either* you must add support *or* you must
disable thread-safety."  Obviously some people will still have problems,
but I suspect that a lot of users don't care about threads, especially
those on more obscure platforms.

I unfortunately don't have time to complete this review (and I no longer
work on protobufs anyway) so I'll leave the rest to Jisi.

BTW, I think there should be an update to the MSVC project files in this
change?  You should be able to make the update manually since the project
files are XML.  Unfortunately if you load them into MSVC it will "upgrade"
them to a newer version which prevents users of older versions from using
them.

On Fri, Feb 17, 2012 at 9:08 AM, <philip.liard@gmail.com> wrote:

> Hi Kenton,
>
> Thanks for reviewing my CL.
>
> I added the flag you mentioned and made the compilation of atomicops.h
> and atomicops_internals_x86_msvc.**cc conditional. For now this change
> does not affect anyone as atomicops is not used (compiled) yet.
>
> However, as you said, we may have to support the non-MSVC/GCC users. The
> flag does not help all of them, as these users are not necessarily the
> ones who don't care about thread safety. The question is what compilers
> do they use/how could we know that?
>
> I will also provide a naive implementation of GoogleOnceInit() in a next
> change covering the case where the flag is defined.
>
> About this set of changes (removing static initializers), I also created
> some internal CLs (Chrome for Android repository) to let us submit the
> change soon (without addressing all the multi-platform issues).
>
> Cheers,
> Philippe.
>
>
> http://codereview.appspot.com/**5654057/diff/7003/src/google/**
>
protobuf/stubs/atomicops.h<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/atomicops.h>
> File src/google/protobuf/stubs/**atomicops.h (right):
>
> http://codereview.appspot.com/**5654057/diff/7003/src/google/**
>
protobuf/stubs/atomicops.h#**newcode54<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/atomicops.h#newcode54>
> src/google/protobuf/stubs/**atomicops.h:54: namespace protobuf {
>
> On 2012/02/17 00:05:54, kenton wrote:
>
>> Please put in google::protobuf::internal, since we don't want clients
>>
> calling
>
>> these.
>>
>
> Done.
>
> http://codereview.appspot.com/**5654057/diff/7003/src/google/**
>
protobuf/stubs/common.h<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/common.h>
> File src/google/protobuf/stubs/**common.h (right):
>
> http://codereview.appspot.com/**5654057/diff/7003/src/google/**
>
protobuf/stubs/common.h#**newcode165<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/common.h#newcode165>
>
> src/google/protobuf/stubs/**common.h:165: typedef int8   int8_t;
> On 2012/02/17 00:05:54, kenton wrote:
>
>> I'm concerned that these typedefs could cause confusion.  First, in
>>
> theory they
>
>> could differ from the ones defined in stdint.h, e.g. int vs. long on
>>
> IA-32 are
>
>> different types of the same size.  Second, you've defined them in the
>>
> protobuf
>
>> namespace, which would lead to inconsistency between MSVC and GCC
>>
> builds of
>
>> protobufs, possibly making maintenance more difficult.
>>
>
>  Could you just change the atomicops code to not use the _t suffix?
>>
>
> Done.
>
> http://codereview.appspot.com/**5654057/diff/7003/src/google/**
>
protobuf/stubs/globals.h<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/globals.h>
> File src/google/protobuf/stubs/**globals.h (right):
>
> http://codereview.appspot.com/**5654057/diff/7003/src/google/**
>
protobuf/stubs/globals.h#**newcode1<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/globals.h#newcode1>
>
> src/google/protobuf/stubs/**globals.h:1: // Copyright 2011 the
> GOOGLE_PROTOBUF project authors. All rights reserved.
> On 2012/02/17 00:05:54, kenton wrote:
>
>> "GOOGLE_PROTOBUF": find/replace error?
>>
>
> Indeed :)
>
> http://codereview.appspot.com/**5654057/diff/7003/src/google/**
>
protobuf/stubs/globals.h#**newcode120<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/globals.h#newcode120>
>
> src/google/protobuf/stubs/**globals.h:120: // Define our own macros for
> writing 64-bit constants.  This is less fragile
> On 2012/02/17 00:05:54, kenton wrote:
>
>> Please delete any macros that aren't used by atomicops.  These ones
>>
> look
>
>> particularly suspicious.
>>
>
> Done.
>
> http://codereview.appspot.com/**5654057/diff/7003/src/google/**
>
protobuf/stubs/globals.h#**newcode156<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/globals.h#newcode156>
> src/google/protobuf/stubs/**globals.h:156: #define USING_BSD_ABI
> On 2012/02/17 00:05:54, kenton wrote:
>
>> Missing GOOGLE_PROTOBUF_.
>>
>
> Done.
>
> http://codereview.appspot.com/**5654057/diff/7003/src/google/**
>
protobuf/stubs/globals.h#**newcode159<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/globals.h#newcode159>
>
> src/google/protobuf/stubs/**globals.h:159: } } // namespace
> google::protobuf
> On 2012/02/17 00:05:54, kenton wrote:
>
>> Please match style of other protobuf source files, here and in other
>>
> files.
>
> I removed the namespace since we renamed the file to platform_macros.h
> which should restrict its content to macros.
>
>
http://codereview.appspot.com/**5654057/<http://codereview.appspot.com/5654057/>
>

[philippe, 2012-02-20 10:39:31]

On 2012/02/17 20:23:57, kenton wrote:
> Regarding other platforms:  My thought was that we can say: "If your
> compiler is not supported, *either* you must add support *or* you must
> disable thread-safety."  Obviously some people will still have problems,
> but I suspect that a lot of users don't care about threads, especially
> those on more obscure platforms.
> 
> I unfortunately don't have time to complete this review (and I no longer
> work on protobufs anyway) so I'll leave the rest to Jisi.
> 
> BTW, I think there should be an update to the MSVC project files in this
> change?  You should be able to make the update manually since the project
> files are XML.  Unfortunately if you load them into MSVC it will "upgrade"
> them to a newer version which prevents users of older versions from using
> them.
> 
> On Fri, Feb 17, 2012 at 9:08 AM, <mailto:philip.liard@gmail.com> wrote:
> 
> > Hi Kenton,
> >
> > Thanks for reviewing my CL.
> >
> > I added the flag you mentioned and made the compilation of atomicops.h
> > and atomicops_internals_x86_msvc.**cc conditional. For now this change
> > does not affect anyone as atomicops is not used (compiled) yet.
> >
> > However, as you said, we may have to support the non-MSVC/GCC users. The
> > flag does not help all of them, as these users are not necessarily the
> > ones who don't care about thread safety. The question is what compilers
> > do they use/how could we know that?
> >
> > I will also provide a naive implementation of GoogleOnceInit() in a next
> > change covering the case where the flag is defined.
> >
> > About this set of changes (removing static initializers), I also created
> > some internal CLs (Chrome for Android repository) to let us submit the
> > change soon (without addressing all the multi-platform issues).
> >
> > Cheers,
> > Philippe.
> >
> >
> > http://codereview.appspot.com/**5654057/diff/7003/src/google/**
> >
>
protobuf/stubs/atomicops.h<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/atomicops.h>
> > File src/google/protobuf/stubs/**atomicops.h (right):
> >
> > http://codereview.appspot.com/**5654057/diff/7003/src/google/**
> >
>
protobuf/stubs/atomicops.h#**newcode54<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/atomicops.h#newcode54>
> > src/google/protobuf/stubs/**atomicops.h:54: namespace protobuf {
> >
> > On 2012/02/17 00:05:54, kenton wrote:
> >
> >> Please put in google::protobuf::internal, since we don't want clients
> >>
> > calling
> >
> >> these.
> >>
> >
> > Done.
> >
> > http://codereview.appspot.com/**5654057/diff/7003/src/google/**
> >
>
protobuf/stubs/common.h<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/common.h>
> > File src/google/protobuf/stubs/**common.h (right):
> >
> > http://codereview.appspot.com/**5654057/diff/7003/src/google/**
> >
>
protobuf/stubs/common.h#**newcode165<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/common.h#newcode165>
> >
> > src/google/protobuf/stubs/**common.h:165: typedef int8   int8_t;
> > On 2012/02/17 00:05:54, kenton wrote:
> >
> >> I'm concerned that these typedefs could cause confusion.  First, in
> >>
> > theory they
> >
> >> could differ from the ones defined in stdint.h, e.g. int vs. long on
> >>
> > IA-32 are
> >
> >> different types of the same size.  Second, you've defined them in the
> >>
> > protobuf
> >
> >> namespace, which would lead to inconsistency between MSVC and GCC
> >>
> > builds of
> >
> >> protobufs, possibly making maintenance more difficult.
> >>
> >
> >  Could you just change the atomicops code to not use the _t suffix?
> >>
> >
> > Done.
> >
> > http://codereview.appspot.com/**5654057/diff/7003/src/google/**
> >
>
protobuf/stubs/globals.h<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/globals.h>
> > File src/google/protobuf/stubs/**globals.h (right):
> >
> > http://codereview.appspot.com/**5654057/diff/7003/src/google/**
> >
>
protobuf/stubs/globals.h#**newcode1<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/globals.h#newcode1>
> >
> > src/google/protobuf/stubs/**globals.h:1: // Copyright 2011 the
> > GOOGLE_PROTOBUF project authors. All rights reserved.
> > On 2012/02/17 00:05:54, kenton wrote:
> >
> >> "GOOGLE_PROTOBUF": find/replace error?
> >>
> >
> > Indeed :)
> >
> > http://codereview.appspot.com/**5654057/diff/7003/src/google/**
> >
>
protobuf/stubs/globals.h#**newcode120<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/globals.h#newcode120>
> >
> > src/google/protobuf/stubs/**globals.h:120: // Define our own macros for
> > writing 64-bit constants.  This is less fragile
> > On 2012/02/17 00:05:54, kenton wrote:
> >
> >> Please delete any macros that aren't used by atomicops.  These ones
> >>
> > look
> >
> >> particularly suspicious.
> >>
> >
> > Done.
> >
> > http://codereview.appspot.com/**5654057/diff/7003/src/google/**
> >
>
protobuf/stubs/globals.h#**newcode156<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/globals.h#newcode156>
> > src/google/protobuf/stubs/**globals.h:156: #define USING_BSD_ABI
> > On 2012/02/17 00:05:54, kenton wrote:
> >
> >> Missing GOOGLE_PROTOBUF_.
> >>
> >
> > Done.
> >
> > http://codereview.appspot.com/**5654057/diff/7003/src/google/**
> >
>
protobuf/stubs/globals.h#**newcode159<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/globals.h#newcode159>
> >
> > src/google/protobuf/stubs/**globals.h:159: } } // namespace
> > google::protobuf
> > On 2012/02/17 00:05:54, kenton wrote:
> >
> >> Please match style of other protobuf source files, here and in other
> >>
> > files.
> >
> > I removed the namespace since we renamed the file to platform_macros.h
> > which should restrict its content to macros.
> >
> >
>
http://codereview.appspot.com/**5654057/%3Chttp://codereview.appspot.com/5654...>
> >

That sounds good.

I updated the visual studio projects.

Thanks

[kenton, 2012-02-22 02:16:39]

(Leaving it to liujisi to pick this up from here.)

On Mon, Feb 20, 2012 at 2:39 AM, <philip.liard@gmail.com> wrote:

> On 2012/02/17 20:23:57, kenton wrote:
>
>> Regarding other platforms:  My thought was that we can say: "If your
>> compiler is not supported, *either* you must add support *or* you must
>> disable thread-safety."  Obviously some people will still have
>>
> problems,
>
>> but I suspect that a lot of users don't care about threads, especially
>> those on more obscure platforms.
>>
>
>  I unfortunately don't have time to complete this review (and I no
>>
> longer
>
>> work on protobufs anyway) so I'll leave the rest to Jisi.
>>
>
>  BTW, I think there should be an update to the MSVC project files in
>>
> this
>
>> change?  You should be able to make the update manually since the
>>
> project
>
>> files are XML.  Unfortunately if you load them into MSVC it will
>>
> "upgrade"
>
>> them to a newer version which prevents users of older versions from
>>
> using
>
>> them.
>>
>
>  On Fri, Feb 17, 2012 at 9:08 AM, <mailto:philip.liard@gmail.com**>
>>
> wrote:
>
>  > Hi Kenton,
>> >
>> > Thanks for reviewing my CL.
>> >
>> > I added the flag you mentioned and made the compilation of
>>
> atomicops.h
>
>> > and atomicops_internals_x86_msvc.****cc conditional. For now this
>>
> change
>
>> > does not affect anyone as atomicops is not used (compiled) yet.
>> >
>> > However, as you said, we may have to support the non-MSVC/GCC users.
>>
> The
>
>> > flag does not help all of them, as these users are not necessarily
>>
> the
>
>> > ones who don't care about thread safety. The question is what
>>
> compilers
>
>> > do they use/how could we know that?
>> >
>> > I will also provide a naive implementation of GoogleOnceInit() in a
>>
> next
>
>> > change covering the case where the flag is defined.
>> >
>> > About this set of changes (removing static initializers), I also
>>
> created
>
>> > some internal CLs (Chrome for Android repository) to let us submit
>>
> the
>
>> > change soon (without addressing all the multi-platform issues).
>> >
>> > Cheers,
>> > Philippe.
>> >
>> >
>> >
http://codereview.appspot.com/****5654057/diff/7003/src/**google/**<http://co...
>> >
>>
>
> protobuf/stubs/atomicops.h<htt**p://codereview.appspot.com/**
>
5654057/diff/7003/src/google/**protobuf/stubs/atomicops.h<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/atomicops.h>
> >
>
>> > File src/google/protobuf/stubs/****atomicops.h (right):
>> >
>> >
http://codereview.appspot.com/****5654057/diff/7003/src/**google/**<http://co...
>> >
>>
>
> protobuf/stubs/atomicops.h#****newcode54<http://codereview.**
> appspot.com/5654057/diff/7003/**src/google/protobuf/stubs/**
>
atomicops.h#newcode54<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/atomicops.h#newcode54>
> >
>
>> > src/google/protobuf/stubs/****atomicops.h:54: namespace protobuf {
>>
>> >
>> > On 2012/02/17 00:05:54, kenton wrote:
>> >
>> >> Please put in google::protobuf::internal, since we don't want
>>
> clients
>
>> >>
>> > calling
>> >
>> >> these.
>> >>
>> >
>> > Done.
>> >
>> >
http://codereview.appspot.com/****5654057/diff/7003/src/**google/**<http://co...
>> >
>>
>
> protobuf/stubs/common.h<http:/**/codereview.appspot.com/**
>
5654057/diff/7003/src/google/**protobuf/stubs/common.h<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/common.h>
> >
>
>> > File src/google/protobuf/stubs/****common.h (right):
>> >
>> >
http://codereview.appspot.com/****5654057/diff/7003/src/**google/**<http://co...
>> >
>>
>
> protobuf/stubs/common.h#****newcode165<http://codereview.**
> appspot.com/5654057/diff/7003/**src/google/protobuf/stubs/**
>
common.h#newcode165<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/common.h#newcode165>
> >
>
>> >
>> > src/google/protobuf/stubs/****common.h:165: typedef int8   int8_t;
>>
>> > On 2012/02/17 00:05:54, kenton wrote:
>> >
>> >> I'm concerned that these typedefs could cause confusion.  First, in
>> >>
>> > theory they
>> >
>> >> could differ from the ones defined in stdint.h, e.g. int vs. long
>>
> on
>
>> >>
>> > IA-32 are
>> >
>> >> different types of the same size.  Second, you've defined them in
>>
> the
>
>> >>
>> > protobuf
>> >
>> >> namespace, which would lead to inconsistency between MSVC and GCC
>> >>
>> > builds of
>> >
>> >> protobufs, possibly making maintenance more difficult.
>> >>
>> >
>> >  Could you just change the atomicops code to not use the _t suffix?
>> >>
>> >
>> > Done.
>> >
>> >
http://codereview.appspot.com/****5654057/diff/7003/src/**google/**<http://co...
>> >
>>
>
> protobuf/stubs/globals.h<http:**//codereview.appspot.com/**
>
5654057/diff/7003/src/google/**protobuf/stubs/globals.h<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/globals.h>
> >
>
>> > File src/google/protobuf/stubs/****globals.h (right):
>> >
>> >
http://codereview.appspot.com/****5654057/diff/7003/src/**google/**<http://co...
>> >
>>
>
> protobuf/stubs/globals.h#****newcode1<http://codereview.**
> appspot.com/5654057/diff/7003/**src/google/protobuf/stubs/**
>
globals.h#newcode1<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/globals.h#newcode1>
> >
>
>> >
>> > src/google/protobuf/stubs/****globals.h:1: // Copyright 2011 the
>>
>> > GOOGLE_PROTOBUF project authors. All rights reserved.
>> > On 2012/02/17 00:05:54, kenton wrote:
>> >
>> >> "GOOGLE_PROTOBUF": find/replace error?
>> >>
>> >
>> > Indeed :)
>> >
>> >
http://codereview.appspot.com/****5654057/diff/7003/src/**google/**<http://co...
>> >
>>
>
> protobuf/stubs/globals.h#****newcode120<http://codereview.**
> appspot.com/5654057/diff/7003/**src/google/protobuf/stubs/**
>
globals.h#newcode120<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/globals.h#newcode120>
> >
>
>> >
>> > src/google/protobuf/stubs/****globals.h:120: // Define our own macros
>>
> for
>
>> > writing 64-bit constants.  This is less fragile
>> > On 2012/02/17 00:05:54, kenton wrote:
>> >
>> >> Please delete any macros that aren't used by atomicops.  These ones
>> >>
>> > look
>> >
>> >> particularly suspicious.
>> >>
>> >
>> > Done.
>> >
>> >
http://codereview.appspot.com/****5654057/diff/7003/src/**google/**<http://co...
>> >
>>
>
> protobuf/stubs/globals.h#****newcode156<http://codereview.**
> appspot.com/5654057/diff/7003/**src/google/protobuf/stubs/**
>
globals.h#newcode156<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/globals.h#newcode156>
> >
>
>> > src/google/protobuf/stubs/****globals.h:156: #define USING_BSD_ABI
>>
>> > On 2012/02/17 00:05:54, kenton wrote:
>> >
>> >> Missing GOOGLE_PROTOBUF_.
>> >>
>> >
>> > Done.
>> >
>> >
http://codereview.appspot.com/****5654057/diff/7003/src/**google/**<http://co...
>> >
>>
>
> protobuf/stubs/globals.h#****newcode159<http://codereview.**
> appspot.com/5654057/diff/7003/**src/google/protobuf/stubs/**
>
globals.h#newcode159<http://codereview.appspot.com/5654057/diff/7003/src/google/protobuf/stubs/globals.h#newcode159>
> >
>
>> >
>> > src/google/protobuf/stubs/****globals.h:159: } } // namespace
>>
>> > google::protobuf
>> > On 2012/02/17 00:05:54, kenton wrote:
>> >
>> >> Please match style of other protobuf source files, here and in
>>
> other
>
>> >>
>> > files.
>> >
>> > I removed the namespace since we renamed the file to
>>
> platform_macros.h
>
>> > which should restrict its content to macros.
>> >
>> >
>>
>
> http://codereview.appspot.com/****5654057/%3Chttp://**
>
codereview.appspot.com/**5654057/<http://codereview.appspot.com/**5654057/%3Chttp://codereview.appspot.com/5654057/>
> >
>
>> >
>>
>
> That sounds good.
>
> I updated the visual studio projects.
>
> Thanks
>
>
http://codereview.appspot.com/**5654057/<http://codereview.appspot.com/5654057/>
>

[Jisi Liu, 2012-02-22 20:01:44]

LGTM

Thanks for the change. Please also update the
http://code.google.com/p/protobuf/source/browse/trunk/vsprojects/extract_incl...
to include those headers.

BTW, maybe mention something in the comments about the upstream(v8)? Do we have
plans integrate new changes from the v8 in the future (e.g. bug fixes, more
platforms, etc)?

[philippe, 2012-02-23 09:04:51]

On 2012/02/22 20:01:44, Pherl Liu wrote:
> LGTM
> 
> Thanks for the change. Please also update the
>
http://code.google.com/p/protobuf/source/browse/trunk/vsprojects/extract_incl...
> to include those headers.
> 
> BTW, maybe mention something in the comments about the upstream(v8)? Do we
have
> plans integrate new changes from the v8 in the future (e.g. bug fixes, more
> platforms, etc)?

Hi Pherl,

Thanks for reviewing this CL.

I updated extract_includes.bat and added a comment in atomicops.h about
upstream. I can maintain the atomicops files here in Protobuf and also the
improved GoogleOnceInit() implementation (currently being integrated and
reviewed in V8) which will follow in the next CL.

[Jisi Liu, 2012-03-01 19:33:06]

LGTM

I added you(pliard@google.com) as a committer to the project, as you committed
to maintain the atomicops in future :) Feel free to submit the change.

On 2012/02/23 09:04:51, philippe wrote:
> On 2012/02/22 20:01:44, Pherl Liu wrote:
> > LGTM
> > 
> > Thanks for the change. Please also update the
> >
>
http://code.google.com/p/protobuf/source/browse/trunk/vsprojects/extract_incl...
> > to include those headers.
> > 
> > BTW, maybe mention something in the comments about the upstream(v8)? Do we
> have
> > plans integrate new changes from the v8 in the future (e.g. bug fixes, more
> > platforms, etc)?
> 
> Hi Pherl,
> 
> Thanks for reviewing this CL.
> 
> I updated extract_includes.bat and added a comment in atomicops.h about
> upstream. I can maintain the atomicops files here in Protobuf and also the
> improved GoogleOnceInit() implementation (currently being integrated and
> reviewed in V8) which will follow in the next CL.

[philippe, 2012-03-02 11:43:52]

On 2012/03/01 19:33:06, Pherl Liu wrote:
> LGTM
> 
> I added mailto:you%28pliard@google.com) as a committer to the project, as you
committed
> to maintain the atomicops in future :) Feel free to submit the change.
> 
> On 2012/02/23 09:04:51, philippe wrote:
> > On 2012/02/22 20:01:44, Pherl Liu wrote:
> > > LGTM
> > > 
> > > Thanks for the change. Please also update the
> > >
> >
>
http://code.google.com/p/protobuf/source/browse/trunk/vsprojects/extract_incl...
> > > to include those headers.
> > > 
> > > BTW, maybe mention something in the comments about the upstream(v8)? Do we
> > have
> > > plans integrate new changes from the v8 in the future (e.g. bug fixes,
more
> > > platforms, etc)?
> > 
> > Hi Pherl,
> > 
> > Thanks for reviewing this CL.
> > 
> > I updated extract_includes.bat and added a comment in atomicops.h about
> > upstream. I can maintain the atomicops files here in Protobuf and also the
> > improved GoogleOnceInit() implementation (currently being integrated and
> > reviewed in V8) which will follow in the next CL.

Great, thanks!

[philippe, 2012-03-02 13:06:28]

On 2012/03/02 11:43:52, philippe wrote:
> On 2012/03/01 19:33:06, Pherl Liu wrote:
> > LGTM
> > 
> > I added mailto:you%28pliard@google.com) as a committer to the project, as
you
> committed
> > to maintain the atomicops in future :) Feel free to submit the change.
> > 
> > On 2012/02/23 09:04:51, philippe wrote:
> > > On 2012/02/22 20:01:44, Pherl Liu wrote:
> > > > LGTM
> > > > 
> > > > Thanks for the change. Please also update the
> > > >
> > >
> >
>
http://code.google.com/p/protobuf/source/browse/trunk/vsprojects/extract_incl...
> > > > to include those headers.
> > > > 
> > > > BTW, maybe mention something in the comments about the upstream(v8)? Do
we
> > > have
> > > > plans integrate new changes from the v8 in the future (e.g. bug fixes,
> more
> > > > platforms, etc)?
> > > 
> > > Hi Pherl,
> > > 
> > > Thanks for reviewing this CL.
> > > 
> > > I updated extract_includes.bat and added a comment in atomicops.h about
> > > upstream. I can maintain the atomicops files here in Protobuf and also the
> > > improved GoogleOnceInit() implementation (currently being integrated and
> > > reviewed in V8) which will follow in the next CL.
> 
> Great, thanks!

Committed as r409. Thanks for the review.