Annotating exported functions

[Darin Fisher]

Ricardo and I are discussing an interesting topic here:

http://codereview.chromium.org/7523051/

It seems like it should probably be moved to the mailing list :)

We're working to break chrome.dll up into a bunch of components (e.g., base.dll, net.dll, media.dll).  As part of this effort, we need to tag classes and functions that should be visible to users of those DLLs.  The set of "users" includes other modules, chrome.dll, and tests.

Each module requires a unique tag (macro) for its exported classes and functions.  This is due to the way symbol exporting works on Windows, in which the macro needs to expand to a different value when building the module versus when using the module.

See base_api.h:

http://codesearch.google.com/#OAMlx_jo-ck/src/base/base_api.h&exact_package=chromium&type=cs

We've defined macros for various top-level projects:  BASE_API, NET_API, etc.

We've also defined a NET_TEST macro, which the net/ module uses to distinguish classes and functions exported only for the benefit of testing.  At first blush, it seems useful to distinguish classes and functions exported for normal consumers from those exported only for tests.  (I find the NET_TEST macro a bit unfortunately named, but try to ignore that for a second.)

However, it occurs to me that if something isn't intended for normal consumers that it really ought to live as part of an internal:: namespace.  That seems like a much clearer way of indicating public versus private.  It has the benefit of requiring people to type internal:: at callsites if they are going to use it, which should hopefully be a strong signal that they probably shouldn't be using that code.

But, here's the rub.  Even if we do make more liberal use of the internal:: namespace, we will still need to apply a tag to export some internal classes and functions for testing purposes.  It may be a bit odd to tag internal things with a NET_API tag since internal stuff is by definition not API.  I've leaned toward accepting this oddity as the concept of "private APIs" is not so foreign to me, but perhaps NET_EXPORT would be better.  Then, we wouldn't need to distinguish exports made for consumers versus exports made for tests, which I think would be a maintenance win.

At any rate, I'd really like some more input from the community on this before we tread further down either path.  Thoughts, suggestions?

-Darin

[Adam Barth]

My read on this topic is that we'll eventually want to grow stronger
separation between these modules so that folks can work on the
relatively independently without needing to understand the complexity
of the whole project.

Symbol exporting is nicely aligned with this isolation between the
compiler helps ensure that folks in other modules don't poke at
internal details. However, an exported symbol is not the same thing
as an API. For example, a virtual function might be part of an API
without being exported, and a symbol might be exported (e.g., for
testing) and not be part of the API.

Those concerns seem to point us towards a name like NET_EXPORT, which
separates the concern of being exported and being an API. We can use
something like an "internal" namespace or a directory of headers to
define what other modules are allowed to poke at. I would start off
using an "internal" namespace and move to stricter controls in the
future if/when we find folks are circumventing the API too much.

Adam

> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev
>

[Evan Martin]

On Fri, Jul 29, 2011 at 3:21 PM, Darin Fisher <da...@chromium.org> wrote:

> But, here's the rub.  Even if we do make more liberal use of the internal::
> namespace, we will still need to apply a tag to export some internal classes
> and functions for testing purposes.

I think we should just link test code in below the API layer. It's a
lot of extra work to go tag every class an function a test might use;
it's reasonable for a test to want to poke at any internal API it
might want to. It also makes the code more verbose. (Yes, there is
value in testing the public API, but there is also value in small,
focused tests.)

My proposal would mean that net.dll and net_unittests would contain
the same code twice. But it is a relatively small amount of code.

[Darin Fisher]

That mean compiling the net code twice on Windows :-(

It also doesn't bode well for making unit_tests and browser_tests link quickly in the future.

-Darin

[Evan Martin]

On Fri, Jul 29, 2011 at 3:59 PM, Darin Fisher <da...@chromium.org> wrote:
> On Fri, Jul 29, 2011 at 3:48 PM, Evan Martin <ev...@chromium.org> wrote:
>>
>> On Fri, Jul 29, 2011 at 3:21 PM, Darin Fisher <da...@chromium.org> wrote:
>> > But, here's the rub.  Even if we do make more liberal use of the
>> > internal::
>> > namespace, we will still need to apply a tag to export some internal
>> > classes
>> > and functions for testing purposes.
>>
>> I think we should just link test code in below the API layer.  It's a
>> lot of extra work to go tag every class an function a test might use;
>> it's reasonable for a test to want to poke at any internal API it
>> might want to.  It also makes the code more verbose.  (Yes, there is
>> value in testing the public API, but there is also value in small,
>> focused tests.)
>>
>> My proposal would mean that net.dll and net_unittests would contain
>> the same code twice.  But it is a relatively small amount of code.
>
> That mean compiling the net code twice on Windows :-(

Compiling, or just linking?

> It also doesn't bode well for making unit_tests and browser_tests link
> quickly in the future.

I guess that depends on how well-factored the related libraries are.
For example, libnet.so is 43mb on Linux and smaller on Windows, and
the tests would bring in some subset of that.

[Darin Fisher]

On Fri, Jul 29, 2011 at 4:03 PM, Evan Martin <ev...@chromium.org> wrote:

On Fri, Jul 29, 2011 at 3:59 PM, Darin Fisher <da...@chromium.org> wrote:
> On Fri, Jul 29, 2011 at 3:48 PM, Evan Martin <ev...@chromium.org> wrote:
>>
>> On Fri, Jul 29, 2011 at 3:21 PM, Darin Fisher <da...@chromium.org> wrote:
>> > But, here's the rub.  Even if we do make more liberal use of the
>> > internal::
>> > namespace, we will still need to apply a tag to export some internal
>> > classes
>> > and functions for testing purposes.
>>
>> I think we should just link test code in below the API layer.  It's a
>> lot of extra work to go tag every class an function a test might use;
>> it's reasonable for a test to want to poke at any internal API it
>> might want to.  It also makes the code more verbose.  (Yes, there is
>> value in testing the public API, but there is also value in small,
>> focused tests.)
>>
>> My proposal would mean that net.dll and net_unittests would contain
>> the same code twice.  But it is a relatively small amount of code.
>
> That mean compiling the net code twice on Windows :-(

Compiling, or just linking?

Compiling.  If you compile the net/ source files with the expectation that

symbols are exported, but then you directly link those object files into

net_unittests, the linker will generate a warning.  Maybe we would just

ignore that warning.

 

> It also doesn't bode well for making unit_tests and browser_tests link
> quickly in the future.

I guess that depends on how well-factored the related libraries are.
For example, libnet.so is 43mb on Linux and smaller on Windows, and
the tests would bring in some subset of that.

I mean, if we have to link all of the code statically into unit_tests as we

do now, then it is going to suck.  The problem is that these executables

are just way too large, and we fall over a cliff.

-Darin

[Dirk Pranke]

On Fri, Jul 29, 2011 at 3:21 PM, Darin Fisher <da...@chromium.org> wrote:

In previous projects the primary way my teams distinguished between
public and private was to put things in different directories in the
filesystem, e.g., you'd have a "src/include" that would contain the
declarations for symbols that could be shared between top-level
components. You could propagate this down into submodules, so that
you'd have "content/include" for stuff shared between
"content/renderer" and "content/browser".

While this was mostly in C, not C++, it seems like that could work
here as well, and I would vastly prefer that to the approach we have
now of people #including files from anywhere in the tree. If your
version control supports it, the files in "src/include" could be
symlinks to the actual files (otherwise the .h won't be next to the
.cc file). Of course, doing any such changes might be fairly invasive
and just not in keeping with the style of this project.

As far as the "private namespace" goes, the downside to moving things
into a different namespace is that you'd have to do a lot of searching
and replacing when you decide to export a function; that seems like
something of a flaw.

I am not troubled by the difference between NET_API and NET_TEST; I
think most people will have no trouble understanding the difference
between something generally available and something only supposed to
be called by other internal code or test code. Really you'd want the
test code to have "package" scope, but we don't have that at the
linker.

-- Dirk

[Peter Kasting]

On Fri, Jul 29, 2011 at 4:10 PM, Dirk Pranke <dpr...@chromium.org> wrote:

In previous projects the primary way my teams distinguished between

public and private was to put things in different directories in the
filesystem, e.g., you'd have a "src/include" that would contain the
declarations for symbols that could be shared between top-level
components.

This seems like it works OK for global functions but not so well when you want to export some members of a class and not others -- you can't break the class declaration into two files.

As far as the "private namespace" goes, the downside to moving things
into a different namespace is that you'd have to do a lot of searching
and replacing when you decide to export a function; that seems like
something of a flaw.

I dunno, that seems OK to me if we want to discourage wanton exporting.

I am not troubled by the difference between NET_API and NET_TEST; I
think most people will have no trouble understanding the difference
between something generally available and something only supposed to
be called by other internal code or test code.

I don't think it's too hard to understand either, but it does have the disadvantage that from the caller side the distinction is invisible -- one needs to look at the callee to distinguish.  Something like "internal::" makes the distinction clear at the caller site which seems like a win to me.

PK 

[Ricardo Vargas]

Another thing to consider is that ideally every class that we have should be unit tested, so unless the tests are performed exclusively through some other class, (for instance an interface), we'll end up moving most of the code to an 'internal' directory and namespace (assuming that we can control the size of the API exported by a module, and we don't end up using everything directly).

The last detail is that most of the time we don't have a real defined interface, and we are just working with what we currently have. For example, things currently annotated with NET_API are not really meant to represent what we want to be the long term API for the net module (which probably will translate to an explicit directory with the set of headers that callers are supposed to use), and instead it mostly documents the current state of things. It is kind of a chicken and egg problem.

--

[Darin Fisher]

[Resuming this thread, and sorry for the wall of text.]

On Fri, Jul 29, 2011 at 6:38 PM, Ricardo Vargas <rva...@chromium.org> wrote:

Another thing to consider is that ideally every class that we have should be unit tested, so unless the tests are performed exclusively through some other class, (for instance an interface), we'll end up moving most of the code to an 'internal' directory and namespace (assuming that we can control the size of the API exported by a module, and we don't end up using everything directly).

The proposal to make more use of the "internal" namespace does not imply moving code into an internal directory.  The point of using this namespace liberally is to allow us to keep the code directory structure that we have, but make it obvious to users what is internal and what is not.

The fact that we should be unit testing nearly everything, and therefore exporting nearly everything, does indeed suggest that a tag like NET_API would be misleading if applied universally.  I agree with that.

 

The last detail is that most of the time we don't have a real defined interface, and we are just working with what we currently have. For example, things currently annotated with NET_API are not really meant to represent what we want to be the long term API for the net module (which probably will translate to an explicit directory with the set of headers that callers are supposed to use), and instead it mostly documents the current state of things. It is kind of a chicken and egg problem.

Yeah, this may also provide motivation for moving away from using NET_API for this purpose.  That may mislead people into thinking that functions and classes annotated with NET_API are indeed intended as stable API.

My proposal is to unify NET_API and NET_TEST into a single NET_EXPORT.  I think we are conflating two things by trying to use these macros to both identify exported functions and distinguish APIs from non-APIs.

To give another concrete example of how xx_API fails, consider a recent change by Nico to a WebKit API interface:  http://trac.webkit.org/changeset/91970

The fix was to apply the WEBKIT_API macro to a private function.  This was done to support a public inline function that is implemented in terms of a private non-inline function.  Clearly, it is odd to annotate a private function with xx_API as private functions are by definition not API.  This motivates me to want to replace WEBKIT_API with WEBKIT_EXPORT.

This inline function example also highlights part of the problem of using these macros to annotate APIs.  Public inline functions can be just as much a part of a public API as a non-inline function, yet inline functions do not need to be exported from a DLL.  So, it is really conflating matters to call these macros xx_API.  (I wish I had realized this issue back when I first typed WEBKIT_API.)

Finally, I have a real problem with NET_TEST.  It doesn't mean what it says.  If you see code like this:

  class NET_TEST HttpVaryData {

   public:

    ...

If I didn't know what HttpVaryData was, and I wasn't super familiar with the meaning of NET_TEST, I'd probably think that this was telling me that HttpVaryData is some kind of test.  But, it is not a test, and indeed, HttpVaryData is a core internal data structure of the Http stack.  A better name for this macro would be NET_EXPORT_FOR_TESTING, but that is a lot to type.  I think NET_EXPORT would be sufficient and less confusing.

I'd like to get some agreement on globally renaming xx_API to xx_EXPORT, and then globally replacing xx_TEST (I think we just have NET_TEST and maybe CONTENT_TEST) with xx_EXPORT.  Make sense?

-Darin

[Jonathan Dixon]

This sounds a good idea to me!

Slightly orthogonal question, but now seems the time to mention it. On a previous system / previous company, rather than have xxx_EXPORT per module, and the per-module header file that entails to define them, we had just two defines used across all modules: IMPORT and EXPORT. Everything in the header file was annotated with IMPORT and all the corresponding bits in the .cc file with EXPORT, and it all seemed to hang together fine. I'm not really up setup to experiment on MSVC these days, but could that work here?

I think the main difference was within the class definitions in the header files each individual function was marked as IMPORT, rather than the class as a whole. Maybe that's the snag?

[Darin Fisher]

On Tue, Aug 2, 2011 at 11:03 AM, Jonathan Dixon <jo...@chromium.org> wrote:

This sounds a good idea to me!

Slightly orthogonal question, but now seems the time to mention it. On a previous system / previous company, rather than have xxx_EXPORT per module, and the per-module header file that entails to define them, we had just two defines used across all modules: IMPORT and EXPORT. Everything in the header file was annotated with IMPORT and all the corresponding bits in the .cc file with EXPORT, and it all seemed to hang together fine. I'm not really up setup to experiment on MSVC these days, but could that work here?

I think the main difference was within the class definitions in the header files each individual function was marked as IMPORT, rather than the class as a whole. Maybe that's the snag?

Hmm, I tried that, and Visual Studio generates a C4273 warning about inconsistent DLL linkage.  It may be fine to suppress that warning.  Not sure.

I think you point out the bigger issue though.  Since we are annotating the class and not the method (in most cases), it won't work.  I think it is nice to annotate the classes since it allows us to have a "lighter touch" on the code base.

-Darin

[Jean-Luc Brouillet]

There's a somewhat relevant article on exporting classes from DLLs on codeproject.com

[Evan Martin]

On Fri, Jul 29, 2011 at 4:05 PM, Darin Fisher <da...@chromium.org> wrote:
> I mean, if we have to link all of the code statically into unit_tests as we
> do now, then it is going to suck.  The problem is that these executables
> are just way too large, and we fall over a cliff.

OK, I am grudgingly convinced. :)

I also like abarth's proposal of using EXPORT for all exported
functions and some separate mechanism for defining public API.

I think needing to tag everything into an "internal" namespace seems a
little too heavy to me. We've currently gone pretty far without
needing to make these barriers so explicit, and I don't see why having
DLLs around means that we need to suddenly force this -- after all,
these API/non-API distinctions matter in a static build too.

By this I mean to propose: use EXPORT for now whenever necessary, and
discuss API boundaries as an orthogonal issue.

[Ricardo Vargas]

On Tue, Aug 2, 2011 at 10:54 AM, Darin Fisher <da...@chromium.org> wrote:

[Resuming this thread, and sorry for the wall of text.]

On Fri, Jul 29, 2011 at 6:38 PM, Ricardo Vargas <rva...@chromium.org> wrote:

Another thing to consider is that ideally every class that we have should be unit tested, so unless the tests are performed exclusively through some other class, (for instance an interface), we'll end up moving most of the code to an 'internal' directory and namespace (assuming that we can control the size of the API exported by a module, and we don't end up using everything directly).

The proposal to make more use of the "internal" namespace does not imply moving code into an internal directory.  The point of using this namespace liberally is to allow us to keep the code directory structure that we have, but make it obvious to users what is internal and what is not.

By looking at the first results of a ::internal code search, I got the impression that the current use of that namespace is to also be directory scoped, as directed by the style guide. Of course we can just decide that this is an exception that we want.

The real reason for NET_TEST to be named that way was to avoid excessive indenting with a more correct name like NET_TEST_API, especially when applied to longer names like CONTENT_. That was probably a bad tradeof.

I think that having MODULE_API for this only makes real sense if there is something else that is not MODULE_API (like MODULE_TEST_API). In other words, if there is  only one annotation it makes more sense for it to reflect what it actually means: an export.

So I'm fine with going to verbose names and a namespace, but we won't have a solution for a class that only exports a method or two.

[Darin Fisher]

On Tue, Aug 2, 2011 at 12:32 PM, Ricardo Vargas <rva...@chromium.org> wrote:

On Tue, Aug 2, 2011 at 10:54 AM, Darin Fisher <da...@chromium.org> wrote:

[Resuming this thread, and sorry for the wall of text.]

On Fri, Jul 29, 2011 at 6:38 PM, Ricardo Vargas <rva...@chromium.org> wrote:

Another thing to consider is that ideally every class that we have should be unit tested, so unless the tests are performed exclusively through some other class, (for instance an interface), we'll end up moving most of the code to an 'internal' directory and namespace (assuming that we can control the size of the API exported by a module, and we don't end up using everything directly).

The proposal to make more use of the "internal" namespace does not imply moving code into an internal directory.  The point of using this namespace liberally is to allow us to keep the code directory structure that we have, but make it obvious to users what is internal and what is not.

By looking at the first results of a ::internal code search, I got the impression that the current use of that namespace is to also be directory scoped, as directed by the style guide. Of course we can just decide that this is an exception that we want.

See weak_ptr.h for example.  It hides the implementation classes in the base::internal:: namespace, exposing only the public classes in the base:: namespace.  I think this works well.

 

The real reason for NET_TEST to be named that way was to avoid excessive indenting with a more correct name like NET_TEST_API, especially when applied to longer names like CONTENT_. That was probably a bad tradeof.

I think that having MODULE_API for this only makes real sense if there is something else that is not MODULE_API (like MODULE_TEST_API). In other words, if there is  only one annotation it makes more sense for it to reflect what it actually means: an export.

OK, I take that to mean that you are OK merging xx_API and xx_TEST into a single xx_EXPORT across the entire project.  Right? :-)

 

So I'm fine with going to verbose names and a namespace, but we won't have a solution for a class that only exports a method or two.

I don't quite understand this last sentence.  Could you clarify?

-Darin

[Darin Fisher]

Yes, I like separating the issues.  By the way, it would be xx_EXPORT instead

of EXPORT.  That is:

BASE_EXPORT

CONTENT_EXPORT 

MEDIA_EXPORT

NET_EXPORT

UI_EXPORT

VIEWS_EXPORT

WEBKIT_EXPORT

...

OKie?

-Darin

[Ami Fischman]

If the annotation no longer marks something as being part of a particular API, but is pure mechanism for dealing with vagaries of build systems, why keep the xx_ prefix and not just use EXPORT?

I.e. to whom is it important that a class is NET_EXPORTed vs. BASE_EXPORTed?

-a

--

[Jói Sigurðsson]

It's important to the compiler (on Windows at least). The export
macros are defined something like this, in e.g. net/net_export.h:

#ifdef NET_MODULE
#define NET_EXPORT __declspec(dllexport)
#else
#define NET_EXPORT __declspec(dllimport)
#endif

Then in your build files, you make it such that for all files in the
net.dll module, NET_MODULE is defined. That makes the compiler/linker
export the symbols for files within the module, and import the symbols
for files outside of the module that use its headers.

I'm not sure you could easily achieve the same thing with just a
single EXPORT define.

Cheers,
Jói

[Ami Fischman]

Thanks for the explanation; I was only familiar w/ the non-windows versions, which use symbol visibility.

-a

[Darin Fisher]

Yeah, if only it worked like linux :-(

The namespaced macro is going to get annoying, especially if we ever have deep modules.  Ben was hopeful that we could carve off the various browser frontends (e.g. chrome/browser/ui/views/) into a separate DLL.  I'm not looking forward to CHROME_BROWSER_UI_VIEWS_EXPORT :-(  We may end up using an acronym!

-Darin

[Ricardo Vargas]

On Tue, Aug 2, 2011 at 12:58 PM, Darin Fisher <da...@chromium.org> wrote:

On Tue, Aug 2, 2011 at 12:32 PM, Ricardo Vargas <rva...@chromium.org> wrote:

On Tue, Aug 2, 2011 at 10:54 AM, Darin Fisher <da...@chromium.org> wrote:

[Resuming this thread, and sorry for the wall of text.]

On Fri, Jul 29, 2011 at 6:38 PM, Ricardo Vargas <rva...@chromium.org> wrote:

Another thing to consider is that ideally every class that we have should be unit tested, so unless the tests are performed exclusively through some other class, (for instance an interface), we'll end up moving most of the code to an 'internal' directory and namespace (assuming that we can control the size of the API exported by a module, and we don't end up using everything directly).

The proposal to make more use of the "internal" namespace does not imply moving code into an internal directory.  The point of using this namespace liberally is to allow us to keep the code directory structure that we have, but make it obvious to users what is internal and what is not.

By looking at the first results of a ::internal code search, I got the impression that the current use of that namespace is to also be directory scoped, as directed by the style guide. Of course we can just decide that this is an exception that we want.

See weak_ptr.h for example.  It hides the implementation classes in the base::internal:: namespace, exposing only the public classes in the base:: namespace.  I think this works well.

 

The real reason for NET_TEST to be named that way was to avoid excessive indenting with a more correct name like NET_TEST_API, especially when applied to longer names like CONTENT_. That was probably a bad tradeof.

I think that having MODULE_API for this only makes real sense if there is something else that is not MODULE_API (like MODULE_TEST_API). In other words, if there is  only one annotation it makes more sense for it to reflect what it actually means: an export.

OK, I take that to mean that you are OK merging xx_API and xx_TEST into a single xx_EXPORT across the entire project.  Right? :-)

I'd be sad to "regress" net to the previous state of no indication at all of what can be used by consumers, but if that's what The People want... 

I know that defining an interface and building a DLL are two separate issues, but this looked like the right time to at least start working in the right direction.

 

So I'm fine with going to verbose names and a namespace, but we won't have a solution for a class that only exports a method or two.

I don't quite understand this last sentence.  Could you clarify?

I was thinking about a class with only a few members exported for one purpose or the other, and being unable to say that some methods are ::internal, but that will only happen if we need both things for the same class: methods exported to consumers and other exported to tests... and I don't think we have/want that, so ignore the comment.

[Darin Fisher]

On Tue, Aug 2, 2011 at 2:16 PM, Ricardo Vargas <rva...@chromium.org> wrote:

On Tue, Aug 2, 2011 at 12:58 PM, Darin Fisher <da...@chromium.org> wrote:

On Tue, Aug 2, 2011 at 12:32 PM, Ricardo Vargas <rva...@chromium.org> wrote:

On Tue, Aug 2, 2011 at 10:54 AM, Darin Fisher <da...@chromium.org> wrote:

[Resuming this thread, and sorry for the wall of text.]

On Fri, Jul 29, 2011 at 6:38 PM, Ricardo Vargas <rva...@chromium.org> wrote:

Another thing to consider is that ideally every class that we have should be unit tested, so unless the tests are performed exclusively through some other class, (for instance an interface), we'll end up moving most of the code to an 'internal' directory and namespace (assuming that we can control the size of the API exported by a module, and we don't end up using everything directly).

The proposal to make more use of the "internal" namespace does not imply moving code into an internal directory.  The point of using this namespace liberally is to allow us to keep the code directory structure that we have, but make it obvious to users what is internal and what is not.

By looking at the first results of a ::internal code search, I got the impression that the current use of that namespace is to also be directory scoped, as directed by the style guide. Of course we can just decide that this is an exception that we want.

See weak_ptr.h for example.  It hides the implementation classes in the base::internal:: namespace, exposing only the public classes in the base:: namespace.  I think this works well.

 

The real reason for NET_TEST to be named that way was to avoid excessive indenting with a more correct name like NET_TEST_API, especially when applied to longer names like CONTENT_. That was probably a bad tradeof.

I think that having MODULE_API for this only makes real sense if there is something else that is not MODULE_API (like MODULE_TEST_API). In other words, if there is  only one annotation it makes more sense for it to reflect what it actually means: an export.

OK, I take that to mean that you are OK merging xx_API and xx_TEST into a single xx_EXPORT across the entire project.  Right? :-)

I'd be sad to "regress" net to the previous state of no indication at all of what can be used by consumers, but if that's what The People want... 

I know that defining an interface and building a DLL are two separate issues, but this looked like the right time to at least start working in the right direction.

I'm really keen to see us move toward a proper API for the net/ module, and I'm sure it was not a trivial amount of work to figure out when to use NET_TEST versus NET_API.  If you prefer, I can map NET_API to NET_EXPORT and NET_TEST to NET_EXPORT_FOR_TESTING.

It looks like NET_TEST is applied to 42 global functions, 128 classes, and 6 structs.  At first glance, expanding NET_TEST to NET_EXPORT_FOR_TESTING will cause a fair bit of extra line wrapping as compared to NET_EXPORT :-(  Even NET_EXPORT is going to cause some more line wrapping.

-Darin

[Jonathan Dixon]

On 2 August 2011 23:24, Darin Fisher <da...@chromium.org> wrote:

I'm really keen to see us move toward a proper API for the net/ module, and I'm sure it was not a trivial amount of work to figure out when to use NET_TEST versus NET_API.  If you prefer, I can map NET_API to NET_EXPORT and NET_TEST to NET_EXPORT_FOR_TESTING.

It looks like NET_TEST is applied to 42 global functions, 128 classes, and 6 structs.  At first glance, expanding NET_TEST to NET_EXPORT_FOR_TESTING will cause a fair bit of extra line wrapping as compared to NET_EXPORT :-(  Even NET_EXPORT is going to cause some more line wrapping.

Maybe eschew the convention and put the FOOBAR_EXPORT on its own line?

NET_EXPORT_FOR_TESTING

class MyClass : public .... {

}

I was also wondering if "EXPORT(NET)" and "TEST_EXPORT(NET)" could be made to work nicely, along the same line as webkit's ENABLE(FEATURE) macro. As the number of modules grows this may help mange the cognitive overhead (I just have to parse it far enough to see it's an export; the module it's exported from is secondary information)

[Jói Sigurðsson]

I think you can pretty easily get the preprocessor to expand
EXPORT(NET) to NET_EXPORT, so yes, this should work. I agree it might
help lower the cognitive overhead.

Cheers,
Jói

[Darin Fisher]

On Wed, Aug 3, 2011 at 3:51 AM, Jonathan Dixon <jo...@chromium.org> wrote:

On 2 August 2011 23:24, Darin Fisher <da...@chromium.org> wrote:

I'm really keen to see us move toward a proper API for the net/ module, and I'm sure it was not a trivial amount of work to figure out when to use NET_TEST versus NET_API.  If you prefer, I can map NET_API to NET_EXPORT and NET_TEST to NET_EXPORT_FOR_TESTING.

It looks like NET_TEST is applied to 42 global functions, 128 classes, and 6 structs.  At first glance, expanding NET_TEST to NET_EXPORT_FOR_TESTING will cause a fair bit of extra line wrapping as compared to NET_EXPORT :-(  Even NET_EXPORT is going to cause some more line wrapping.

Maybe eschew the convention and put the FOOBAR_EXPORT on its own line?

NET_EXPORT_FOR_TESTING

class MyClass : public .... {

}

Yeah, perhaps.

 

I was also wondering if "EXPORT(NET)" and "TEST_EXPORT(NET)" could be made to work nicely, along the same line as webkit's ENABLE(FEATURE) macro. As the number of modules grows this may help mange the cognitive overhead (I just have to parse it far enough to see it's an export; the module it's exported from is secondary information)

Interesting idea.  I need to think about it some more.

-Darin

 

[Dirk Pranke]

off-list, to you two ...

On Fri, Jul 29, 2011 at 4:48 PM, Peter Kasting <pkas...@chromium.org> wrote:

> On Fri, Jul 29, 2011 at 4:10 PM, Dirk Pranke <dpr...@chromium.org> wrote:
>>
>> In previous projects the primary way my teams distinguished between
>> public and private was to put things in different directories in the
>> filesystem, e.g., you'd have a "src/include" that would contain the
>> declarations for symbols that could be shared between top-level
>> components.
>
> This seems like it works OK for global functions but not so well when you
> want to export some members of a class and not others -- you can't break the
> class declaration into two files.

This is a fair point. However, I would generally think that it's a bad
design to have public members of the same class that have different
levels of visibility (true public vs. only available inside a
component, i.e., like java's "package" scope.), especially as you move
higher up the stack. I could easily be wrong though, for performance
concerns (or maybe simple design reasons). Are there many examples
where we need/want this? I'd be curious to look at them.

-- Dirk

[Dirk Pranke]

Okay, I failed the offlist part :( . Feel free to reply off- or on-list.

-- Dirk

[Darin Fisher]

On Mon, Aug 8, 2011 at 3:43 PM, Dirk Pranke <dpr...@google.com> wrote:

I know of an example of this.  See NativeTextfieldWin.  There is an intention to

make that class be an implementation detail of src/views/, but outside of src/views/

there are still a couple methods that get used.  You can see that instead of exporting

the entire class, I just exported the two methods.  Eventually, the intention is to stop

calling them, and then the class can be entirely hidden.

That said, I think you were asking about cases where we would want to design an

API that is split like this.  WebKit API has some of this.  You can see that the not

intended for external consumption parts are wrapped with WEBKIT_IMPLEMENTATION.

-Darin

[Dirk Pranke]

I don't think we actually reached a conclusion on this thread, but
off-thread, we converged on a single COMPONENT_EXPORT macro and using
either explicit naming or an internal namespace to segregate symbols
exposed solely for testing.

I have updated the coding standards accordingly:

https://sites.google.com/a/chromium.org/dev/developers/coding-style?pli=1#TOC-Exporting-Symbols

-- Dirk

[Darin Fisher]

Thanks Dirk!  Also, all of the FOO_API macros have been converted to FOO_EXPORT.

-Darin