You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@qpid.apache.org by Justin Ross <ju...@gmail.com> on 2015/01/29 16:05:02 UTC
Simplify Proton C includes?
The Proton C code has a lot of redundant includes. That is, headers often
directly include items they already have transitively. This makes some of
the dependency graphs in our api doc less attractive and less helpful:
http://qpid.apache.org/releases/qpid-proton-0.8/protocol-engine/c/api/connection_8h.html
http://qpid.apache.org/releases/qpid-proton-0.8/protocol-engine/c/api/message_8h.html
http://qpid.apache.org/releases/qpid-proton-0.8/protocol-engine/c/api/messenger_8h.html
(this is a really good one)
http://qpid.apache.org/releases/qpid-proton-0.8/protocol-engine/c/api/transport_8h.html
If there's no technical objection, I'd like to produce a patch to remove
the unnecessary includes. Are there any platform or compiler issues I
should be aware of?
Justin
Re: Simplify Proton C includes?
Posted by Andrew Stitcher <as...@redhat.com>.
On Thu, 2015-01-29 at 11:43 -0500, Justin Ross wrote:
> ...
> > This makes the file robust against any change of transitive includes
>
> because the dependencies of its dependencies change.
We call this refactoring, and we should be doing it all the time.
> >
>
> At the obviously quite tremendous risk of speaking from too little
> experience with C projects, this seems too simple. *If you don't control
> the header dependencies of a third party project*, I agree it makes sense
> to code defensively.
>
> But if it's inside your own source project, those redundant includes aren't
> really defending you against anything.
I don't think this is defensive coding - it is correct coding - it means
that every header file stands alone irrespective of changes in its
dependencies. It makes the code better decoupled at the "physical"
structure level and makes changes to the code better localised, which is
strongly beneficial even if you control all the source.
In any case there should be very few places where this actually comes up
to be honest. It should only come up with headers that define types that
are always used as values and not references. So I'd expect something
like proton/types.h (or whatever it's actually called) to be used in
nearly every header, but proton/transport.h to be used hardly anywhere
(nowhere?) since the types it defines are always used as references.
I think that starting with a picture in the documentation that you think
is confusing and then deciding to change the structure of the includes
to simplify the picture is putting the cart before the horse.
Of course if the picture genuinely represent a bad project structure
then we should change that structure, otherwise just omit the picture,
or use directives to simplify irrelevant bits of it (assuming these
exist).
Andrew
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: Simplify Proton C includes?
Posted by Justin Ross <ju...@gmail.com>.
On Thu, Jan 29, 2015 at 11:27 AM, Andrew Stitcher <as...@redhat.com>
wrote:
>
> The current best practice in C (and C++) is for a header file (and a
> source file too) to include any header that defines a type that the file
> directly depends on (irrespective of any transitive includes).
> This makes the file robust against any change of transitive includes
because the dependencies of its dependencies change.
>
At the obviously quite tremendous risk of speaking from too little
experience with C projects, this seems too simple. *If you don't control
the header dependencies of a third party project*, I agree it makes sense
to code defensively.
But if it's inside your own source project, those redundant includes aren't
really defending you against anything.
(Though the import model is of course quite different, this same issue
comes up in python projects.)
Re: Simplify Proton C includes?
Posted by Andrew Stitcher <as...@redhat.com>.
On Thu, 2015-01-29 at 12:12 -0500, Rafael Schloming wrote:
> ...
>
> Hmm, should we consider adding this to our build? I know I've fairly
> recently fixed a bunch of bugs where I include a file and it breaks because
> it's only ever been included along with some other file that supplied
> something it needed.
Generally the way to do this is to make sure that one file (usually the
file that implements the functions in the header) includes the header
before any other includes. That way if the header does depend on any
other headers accidentally then it will error out then.
So for example in proton/foo.c there would be:
/* License.....
*/
#include "proton/foo.h"
... Other headers
... Implenetations
This should catch most of the headers, but obviously not the ones that
don't have a specific implementation c file.
Andrew
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: Simplify Proton C includes?
Posted by Rafael Schloming <rh...@alum.mit.edu>.
On Thu, Jan 29, 2015 at 11:27 AM, Andrew Stitcher <as...@redhat.com>
wrote:
> On Thu, 2015-01-29 at 10:05 -0500, Justin Ross wrote:
> > The Proton C code has a lot of redundant includes. That is, headers
> often
> > directly include items they already have transitively. This makes some
> of
> > the dependency graphs in our api doc less attractive and less helpful:
> >
> I think this is a problem with the doc generator mostly, not the actual
> header files.
>
> The current best practice in C (and C++) is for a header file (and a
> source file too) to include any header that defines a type that the file
> directly depends on (irrespective of any transitive includes).
>
> This makes the file robust against any change of transitive includes
> because the dependencies of its dependencies change.
>
> So I agree with Alan, we should remove _unnecessary_ includes but not
> ones that are redundant because of transitive includes.
>
> Another point is that you should only need to include a header file for
> a type if you are using details of the type's structure not if you
> merely use a pointer to that type in an API - that can be covered with a
> forward declaration of the type like "typedef struct pn_foo pn_foo;".
>
> In general the C headers should have few reasons to need the details of
> other complex types.
>
> One reason why C headers very often do not follow these rules is that
> people usually just compile source files and generally do not check that
> header files compile on their own.
>
Hmm, should we consider adding this to our build? I know I've fairly
recently fixed a bunch of bugs where I include a file and it breaks because
it's only ever been included along with some other file that supplied
something it needed.
--Rafael
Re: Simplify Proton C includes?
Posted by Andrew Stitcher <as...@redhat.com>.
On Thu, 2015-01-29 at 10:05 -0500, Justin Ross wrote:
> The Proton C code has a lot of redundant includes. That is, headers often
> directly include items they already have transitively. This makes some of
> the dependency graphs in our api doc less attractive and less helpful:
>
I think this is a problem with the doc generator mostly, not the actual
header files.
The current best practice in C (and C++) is for a header file (and a
source file too) to include any header that defines a type that the file
directly depends on (irrespective of any transitive includes).
This makes the file robust against any change of transitive includes
because the dependencies of its dependencies change.
So I agree with Alan, we should remove _unnecessary_ includes but not
ones that are redundant because of transitive includes.
Another point is that you should only need to include a header file for
a type if you are using details of the type's structure not if you
merely use a pointer to that type in an API - that can be covered with a
forward declaration of the type like "typedef struct pn_foo pn_foo;".
In general the C headers should have few reasons to need the details of
other complex types.
One reason why C headers very often do not follow these rules is that
people usually just compile source files and generally do not check that
header files compile on their own.
Andrew
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org
Re: Simplify Proton C includes?
Posted by Alan Conway <ac...@redhat.com>.
On Thu, 2015-01-29 at 10:05 -0500, Justin Ross wrote:
> The Proton C code has a lot of redundant includes. That is, headers often
> directly include items they already have transitively. This makes some of
> the dependency graphs in our api doc less attractive and less helpful:
>
>
> http://qpid.apache.org/releases/qpid-proton-0.8/protocol-engine/c/api/connection_8h.html
>
> http://qpid.apache.org/releases/qpid-proton-0.8/protocol-engine/c/api/message_8h.html
>
> http://qpid.apache.org/releases/qpid-proton-0.8/protocol-engine/c/api/messenger_8h.html
> (this is a really good one)
>
> http://qpid.apache.org/releases/qpid-proton-0.8/protocol-engine/c/api/transport_8h.html
>
> If there's no technical objection, I'd like to produce a patch to remove
> the unnecessary includes. Are there any platform or compiler issues I
> should be aware of?
+1 for redundant (already transitively included) headers.
Just beware if you get more enthusiastic: removing un-necessary (as
opposed to redundant) includes is source incompatible for .c files that
rely on getting the included .h indirectly.
In C++ un-necessary includes can be a surprisingly large chunk of the
build time, but in C on a project of proton's size this probably isn't
important.
Cheers,
Alan.
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org