linux: components support for base/

linux: components support for base/

This builds base_unittests using libbase as a component.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=83583

[Lei Zhang, 2011-04-28 23:21:45]

I defer to rvargas.

http://codereview.chromium.org/6904109/diff/1/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/6904109/diff/1/base/base.gypi#newcode544
base/base.gypi:544: 'direct_dependent_settings': {
Do we need to propagate the BASE_DLL define for OS!="win" ?

http://codereview.chromium.org/6904109/diff/1/base/string16.h
File base/string16.h (right):

http://codereview.chromium.org/6904109/diff/1/base/string16.h#newcode34
base/string16.h:34: #include "base/base_api.h"
nit: base before basic.

[Evan Martin, 2011-04-28 23:24:52]

I included rvargas more as an FYI.  With that in mind, do you think I find
another reviewer?

http://codereview.chromium.org/6904109/diff/1/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/6904109/diff/1/base/base.gypi#newcode544
base/base.gypi:544: 'direct_dependent_settings': {
On 2011/04/28 23:21:45, Lei Zhang wrote:
> Do we need to propagate the BASE_DLL define for OS!="win" ?

No, it's used for dllimport on Win but it doesn't do anything on non-Win.

[rvargas (doing something else), 2011-04-28 23:29:43]

http://codereview.chromium.org/6904109/diff/1/base/synchronization/lock_impl.h
File base/synchronization/lock_impl.h (right):

http://codereview.chromium.org/6904109/diff/1/base/synchronization/lock_impl....
base/synchronization/lock_impl.h:10: #include "base/basictypes.h"
I think we still want these to go after the os dependent block given that
<windows.h> is included there. Granted, there should not be an issue with what
we have here, but the next chromium include added may have issues.

http://codereview.chromium.org/6904109/diff/1/base/synchronization/lock_impl....
base/synchronization/lock_impl.h:25: class BASE_API LockImpl {
Why do we need to export this? It should not be used outside base.

http://codereview.chromium.org/6904109/diff/4002/base/at_exit.h
File base/at_exit.h (right):

http://codereview.chromium.org/6904109/diff/4002/base/at_exit.h#newcode72
base/at_exit.h:72: class BASE_API ShadowingAtExitManager : public AtExitManager
{
Why is this needed? There is no implementation outside this header.

[Evan Martin, 2011-04-28 23:39:40]

PTAL

http://codereview.chromium.org/6904109/diff/1/base/synchronization/lock_impl.h
File base/synchronization/lock_impl.h (right):

http://codereview.chromium.org/6904109/diff/1/base/synchronization/lock_impl....
base/synchronization/lock_impl.h:10: #include "base/basictypes.h"
On 2011/04/28 23:29:43, rvargas wrote:
> I think we still want these to go after the os dependent block given that
> <windows.h> is included there. Granted, there should not be an issue with what
> we have here, but the next chromium include added may have issues.

There was some thread on chromium-dev where we decided to always put these
before OS-specific headers, so I think we're ok, but I changed it back just in
case because this isn't relevant to my change.

http://codereview.chromium.org/6904109/diff/1/base/synchronization/lock_impl....
base/synchronization/lock_impl.h:25: class BASE_API LockImpl {
On 2011/04/28 23:29:43, rvargas wrote:
> Why do we need to export this? It should not be used outside base.

Lock::~Lock() is inlined, and it calls LockImpl::~LockImpl.


/work/chrome/bin/ld: ninja/obj/base_unittests/base/observer_list_unittest.o: in
function base::Lock::~Lock():./base/synchronization/lock.h:36: error: undefined
reference to 'base::internal::LockImpl::~LockImpl()'

http://codereview.chromium.org/6904109/diff/4002/base/at_exit.h
File base/at_exit.h (right):

http://codereview.chromium.org/6904109/diff/4002/base/at_exit.h#newcode72
base/at_exit.h:72: class BASE_API ShadowingAtExitManager : public AtExitManager
{
On 2011/04/28 23:29:44, rvargas wrote:
> Why is this needed? There is no implementation outside this header.

Fixed.

[rvargas (doing something else), 2011-04-28 23:45:31]

LGTM

http://codereview.chromium.org/6904109/diff/1/base/synchronization/lock_impl.h
File base/synchronization/lock_impl.h (right):

http://codereview.chromium.org/6904109/diff/1/base/synchronization/lock_impl....
base/synchronization/lock_impl.h:10: #include "base/basictypes.h"
On 2011/04/28 23:39:40, Evan Martin wrote:
> On 2011/04/28 23:29:43, rvargas wrote:
> > I think we still want these to go after the os dependent block given that
> > <windows.h> is included there. Granted, there should not be an issue with
what
> > we have here, but the next chromium include added may have issues.
> 
> There was some thread on chromium-dev where we decided to always put these
> before OS-specific headers, so I think we're ok, but I changed it back just in
> case because this isn't relevant to my change.

Yes, that same thread mentioned this exception (although it is not in the style
guide :( ).