Base: First pass at having base.dll: definition of

Base: First pass at having base.dll: definition of
BASE_API and a few files that use it.

BUG=76996
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79056

[brettw, 2011-03-22 21:35:33]

LGTM

[wtc, 2011-03-23 22:05:12]

rvargas: I have some questions about this CL.

http://codereview.chromium.org/6725001/diff/1/base/base_api.h
File base/base_api.h (right):

http://codereview.chromium.org/6725001/diff/1/base/base_api.h#newcode10
base/base_api.h:10: #define BASE_IMPLEMENTATION 0
I think we should not define BASE_IMPLEMENTATION as 0.
We should leave BASE_IMPLEMENTATION undefined, and change
the test on line 14 from
  #if BASE_IMPLEMENTATION
to
  #if defined(BASE_IMPLEMENTATION)

Where do you add -DBASE_IMPLEMENTATION to the compiler
command line?

http://codereview.chromium.org/6725001/diff/1/base/base_api.h#newcode13
base/base_api.h:13: #if defined(WIN32) && defined(BASE_DLL)
Nit: I think we prefer our own OS_WIN macro over WIN32.

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

http://codereview.chromium.org/6725001/diff/1/base/synchronization/lock.h#new...
base/synchronization/lock.h:18: class BASE_API Lock {
Can you summarize the rules for determining which classes
and functions need BASE_API.

I noticed that inline functions do not need BASE_API.

For classes, I don't know what the rules are.  This file
is a good example.  Why don't the AutoLock and AutoUnlock
classes need BASE_API?  Is it also because they can be
inlined?

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

http://codereview.chromium.org/6725001/diff/1/base/synchronization/lock_impl....
base/synchronization/lock_impl.h:26: class BASE_API LockImpl {
Why does an internal class LockImpl need BASE_API?
The exported class Lock only contains a pointer to LockImpl.

[rvargas (doing something else), 2011-03-23 22:59:04]

Thanks.

http://codereview.chromium.org/6725001/diff/1/base/base_api.h
File base/base_api.h (right):

http://codereview.chromium.org/6725001/diff/1/base/base_api.h#newcode10
base/base_api.h:10: #define BASE_IMPLEMENTATION 0
On 2011/03/23 22:05:12, wtc wrote:
> I think we should not define BASE_IMPLEMENTATION as 0.
> We should leave BASE_IMPLEMENTATION undefined, and change
> the test on line 14 from
>   #if BASE_IMPLEMENTATION
> to
>   #if defined(BASE_IMPLEMENTATION)
> 
> Where do you add -DBASE_IMPLEMENTATION to the compiler
> command line?

BASE_DLL and BASE_IMPLEMENTATION are defined on the gyp file that generates the
project files, but that is the last step, after everything else is ready.

To be honest, I just copied the pattern used by the equivalent file in
webkit/v8/gurl. I would have also written this without defining BASE_IMPL to 0,
but I see some value in having the exact same pattern that we use in other
places.

http://codereview.chromium.org/6725001/diff/1/base/base_api.h#newcode13
base/base_api.h:13: #if defined(WIN32) && defined(BASE_DLL)
On 2011/03/23 22:05:12, wtc wrote:
> Nit: I think we prefer our own OS_WIN macro over WIN32.

That's an interesting point, but in order to do that, this file would have to
include base/build_config.h, and that would muddy the waters a bit.

I think it's better to have a very simple header that doesn't depend on any
other header from the module that it's trying to define the interface for. There
is also the point that this whole exercise is Windows-only.

But, I could be convinced about changing the structure of this file :)

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

http://codereview.chromium.org/6725001/diff/1/base/synchronization/lock.h#new...
base/synchronization/lock.h:18: class BASE_API Lock {
On 2011/03/23 22:05:12, wtc wrote:
> Can you summarize the rules for determining which classes
> and functions need BASE_API.
> 
> I noticed that inline functions do not need BASE_API.
> 
> For classes, I don't know what the rules are.  This file
> is a good example.  Why don't the AutoLock and AutoUnlock
> classes need BASE_API?  Is it also because they can be
> inlined?

The basic rule is that anything that is not directly visible to the compiler has
to be exported. In other words, code implemented on cc files, or on files that
are not directly included.

Exporting a class exposes all public members, but not the private ones. Classes
defined as part of the public interface of a class also have to be exported.

At the end of the day, there may be cases where the unit test requires more
stuff than really needed (internal details), or is not testing parts of the code
that also have to be exported when they are used by other modules; but we'll
only find out about that when trying to build everything.

In the AutoLock example, we'll end up with multiple versions of the code, one on
each module (base/net/chrome etc). As long as there are no static members (or
there's no real issue with having multiple copies on the same process),
everything should be fine.

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

http://codereview.chromium.org/6725001/diff/1/base/synchronization/lock_impl....
base/synchronization/lock_impl.h:26: class BASE_API LockImpl {
On 2011/03/23 22:05:12, wtc wrote:
> Why does an internal class LockImpl need BASE_API?
> The exported class Lock only contains a pointer to LockImpl.

I swear I tried without modifying this file and the linker complained (for
net_unittests). But I just tried again and it worked fine :(. It could have been
because most of this patch is quite old; I originally did this with the code
from early January, and maybe there was an extra dependency.

Anyways, I'll remove this on another CL.

[wtc, 2011-03-25 18:17:29]

http://codereview.chromium.org/6725001/diff/1/base/base_api.h
File base/base_api.h (right):

http://codereview.chromium.org/6725001/diff/1/base/base_api.h#newcode14
base/base_api.h:14: #if BASE_IMPLEMENTATION
Thank you for the reply.  I agree base_api.h should not
need to include build_config.h.  So we should only test
compiler-predefined macros or macros defined on the compiler
command line.

I think we should remove lines 9-11 and test
#if defined(BASE_IMPLEMENTATION)
here.  Lines 9-11 seem to show we don't know the
C preprocessor replaces an undefined macro with the
value 0L in a conditional expression.  We can change
WebKit and googleurl if we want consistency.

[brettw, 2011-03-26 18:09:46]

http://codereview.chromium.org/6725001/diff/1/base/base_api.h
File base/base_api.h (right):

http://codereview.chromium.org/6725001/diff/1/base/base_api.h#newcode14
base/base_api.h:14: #if BASE_IMPLEMENTATION
This sounds fine to me.

[rvargas (doing something else), 2011-03-28 18:52:21]

Thanks again.

http://codereview.chromium.org/6725001/diff/1/base/base_api.h
File base/base_api.h (right):

http://codereview.chromium.org/6725001/diff/1/base/base_api.h#newcode14
base/base_api.h:14: #if BASE_IMPLEMENTATION
On 2011/03/26 18:09:46, brettw wrote:
> This sounds fine to me.

Yeah. A CL with this change is up for review
(http://codereview.chromium.org/6747014/).