order of includes and include protections

[rcurtin]

Reported by nslagle on 8 Aug 41823682 22:04 UTC
A little nitpicky, but I prefer includes appearing in a file to appear in four blocks:

#include <iostream>  // BLOCK I: standard includes available on virtually all 
#include <fstream>   //   systems requiring no additional libraries
#include <stdlib.h>

#include <armadillo> // BLOCK II: includes from our installed dependencies
#include <boost/booster_seat/seatbelt.hpp>
#include <libxml/parser.h>

#include <mlpack/core/core.hpp> // BLOCK III: our local library includes
#include <mlpack/core/tree/spacetree.hpp>

#include "my_local_class_definition.hpp" // BLOCK IV: my locally required headers

In each separate block, alphabetize the listing by header name. Also, headers appearing in block IV should appear using quotes to indicate a ''lesser'' status, if you will. Of course, this ''lesser'' status within this file might be (and hopefully is) a higher status in other more distant source files.

By our templating convention, we create the intermediary ''do_stuff_ever_impl.hpp'' file to house complicated template methods for the class ''DoStuffEver''. We typically include this implementation intermediary at the conclusion of the top header file for the class.

#ifndef DO_STUFF_EVER_HPP
#define DO_STUFF_EVER_HPP

class DoStuffEver
{
...
};

#include "do_stuff_ever_impl.hpp"
#endif

This type of include falls under ''BLOCK IV'', so use ordinary double quotes rather than angled brackets.

Finally, in the file ''do_stuff_ever_impl.hpp'', do not use any ''#includes''; simply begin the file with

#ifndef DO_STUFF_EVER_HPP
#error "Do not include this file directly."
#endif

#ifndef DO_STUFF_EVER_IMPL_HPP
#define DO_STUFF_EVER_IMPL_HPP
...
#endif

Restricting this file to contain no additional includes further protects us from including the file improperly; any ''includes'' required for this file can appear in ''do_stuff_ever.hpp'' above.

[rcurtin]

Commented by nslagle on 4 Oct 41823686 05:04 UTC
We should place some of this in the style guide, pending consensus.

[rcurtin]

Commented by nslagle on 1 Apr 41825626 14:06 UTC
A better way to guard against improper inclusion is to define a macro in the standard ''.hpp'' as follows:

#ifndef DO_STUFF_EVER_HPP
#define DO_STUFF_EVER_HPP

class DoStuffEver
{
...
};

#define USE_DO_STUFF_EVER_IMPL
#include "do_stuff_ever_impl.hpp"
#undef USE_DO_STUFF_EVER_IMPL
#endif

In the ''do_stuff_ever_impl.hpp'', add a check for the single use ''#define'' from above:

#ifndef DO_STUFF_EVER_HPP
#ifndef USE_DO_STUFF_EVER_IMPL
#error "Do not include this file directly."
#endif
#endif

#ifndef DO_STUFF_EVER_IMPL_HPP
#define DO_STUFF_EVER_IMPL_HPP
...
#endif

[rcurtin]

Commented by rcurtin on 15 May 41833892 12:59 UTC
I don't think the extra complexity of the USE_DO_STUFF_EVER_IMPL is necessary; I think we should assume that if DO_STUFF_EVER_HPP is defined, then do_stuff_ever.hpp was included (at some point) and it's safe to include the implementation.

If the user is brash enough to define DO_STUFF_EVER_HPP on their own, well, then, I say, we let them be as dangerous as they like. One doesn't arbitrarily write #define DO_STUFF_EVER_HPP without knowing what they're doing -- and if they really are that clueless, they're beyond our help.

Also, I'll be picky and say that the actual format of the include guards should be __MLPACK_PATH_TO_FILE_HPP; such as, __MLPACK_METHODS_NCA_NCA_HPP or similar. At least, that's mostly how we're doing it already.

I've started to implement your original solution for protecting _impl.hpp files.

[rcurtin]

Commented by rcurtin on 31 Mar 41833893 06:09 UTC
Er, meant to include a line in there asking if ditching the USE_DO_STUFF_EVER_IMPL macro is reasonable.

[rcurtin]

Commented by nslagle on 10 Sep 41833980 17:04 UTC
I suggested the additional protection to prevent direct inclusion altogether of ''do_stuff_ever_impl.hpp''; that is, a coder shouldn't ever directly include this header. Using a single layer of protection doesn't guard against direct inclusion since, as you point out, they might include ''do_stuff_ever.hpp'' somewhere above, freeing them to directly include the ''_impl.hpp'' file even though they ought not.

The extra layer forces a user to define an otherwise undefined macro before using it, at which point they'd see the prohibition statement in ''#error''.

[rcurtin]

Commented by rcurtin on 13 Feb 41834151 07:18 UTC
But the second layer of protection seems superfluous to me and just makes it more irritating for us to write .hpp and _impl.hpp files. If a user, somewhere, includes file.hpp, then file.hpp will (always) include file_impl.hpp. If they try to re-include file_impl.hpp by hand, it doesn't make a difference because of the standard include guards, and all our #error does is show them that they're kind of stupid. But in my opinion, we shouldn't go to such lengths to tell people they're dumb.

In other words, I can't see a possible case where the simpler approach fails to prevent unwanted behavior. The only issue we need to catch is where someone includes file_impl.hpp before file.hpp, or, when they include file_impl.hpp but not file.hpp. The simple approach catches that in every case.

[rcurtin]

Commented by jcline3 on 30 Jan 41834267 12:50 UTC
This feels like a solution looking for a problem. It seems entirely correct for our users to #include and nothing else. This shouldn't even be an issue.

If you're worried about people adding to mlpack itself, they should just include our core.h, and that should pull in all the pieces they need. Any other includes they have would be for the things they are specifically working on, not features already in the library, which should be included via core.h.

[rcurtin]

Commented by nslagle on 24 Jun 41834991 01:58 UTC
My suggestion to add the additional check isn't really intended to caricature users; when I approached this library early in the summer, I'd never seen ''stuff_impl.h'' files in addition to ''stuff.hpp.'' Additional code, however frustrating adding four lines to code might be, is well-worth it in encouraging proper usage.

Another solution adopted by others in the C++ template camp is to use a different extension for the ''stuff_impl.hpp'' file, perhaps ''stuff.impl'' (I can't remember the suffix exactly.)

@jc: Is our solution to simply provide one gigantic include for the entire library, including all of the methods? I see that armadillo does something similar with its global include, though I'm not sure about doing this in production code, at least not internally.

''An advantage in reducing your include dependencies to the minimum necessary to accomplish the task is to ease understanding of the dependencies.''

Namespaces ostensibly help us with this, but we often mitigate this with ''using namespace ohWell;''. Admittedly, we haven't the time right now to discover exactly which armadillo dependencies we're using (though likely we aren't using very many of them), but we don't necessarily have to follow the same sloppy paradigm, at least not internally.

Take KDE for instance; I don't really need everything available in core.h, so probably I can just add three to five internal includes, and anyone looking at the code immediately understands the dependencies. Thus, the curious reader can look at the KDE header to see that ''spacetree.hpp, hrectbound.hpp, range.hpp,'' and ''log.hpp'' are necessary to make it work.

Externally, for the babes in the woods, we can provide the all-inclusive-every-header-on-the-system approach, though that still seems like overkill to me.

The rule we followed in developing simulator C code is to blob together includes only when they form a chain of dependencies necessary to make the final target run (much like our ''do_stuff.hpp'' and ''do_stuff_impl.hpp'' dependencies.)

[rcurtin]

Commented by rcurtin on 3 Apr 41835037 15:27 UTC

Another solution adopted by others in the C++ template camp is to use a different extension for the stuff_impl.hpp file, perhaps stuff.impl (I can't remember the suffix exactly.)

I've never seen that; the _impl.hpp or _impl.h is fine with me. I don't see what problem that would solve.

I'd never seen stuff_impl.h files in addition to stuff.hpp. Additional code, however frustrating adding four lines to code might be, is well-worth it in encouraging proper usage.

This is reasonable:

/** some_file_impl.hpp */
#ifndef SOME_FILE_HPP
#error "Include some_file.hpp, not some_file_impl.hpp."
#endif

#ifndef SOME_FILE_IMPL_HPP
#define SOME_FILE_IMPL_HPP
...
#endif

But this is superfluous:

/** some_file_impl.hpp */
#ifndef SOME_FILE_HPP
#ifndef USE_SOME_FILE_HPP
#error "Include some_file.hpp, not some_file_impl.hpp."
#endif
#endif

#ifndef SOME_FILE_IMPL_HPP
#define SOME_FILE_IMPL_HPP
...
#endif

And, like I stated earlier, this is why:

If someone is dumb enough to define SOME_FILE_HPP but not include some_file.hpp, then there is no helping them. It's not our problem.

And none of this is about encouraging proper usage. You don't want someone to include some_file_impl.hpp because there may be declarations in some_file.hpp which aren't present in some_file_impl.hpp (such as an inline function) and now everything breaks. In fact there's even a third solution which is even simpler:

/** some_file_impl.hpp */
#ifndef SOME_FILE_IMPL_HPP
#define SOME_FILE_IMPL_HPP

// Just in case someone included this directly...
#include "some_file.hpp"

#endif

It isn't our problem if our users our stupid, but it is our problem if our library is counterintuitive and poorly designed. In my opinion the USE_SOME_FILE_HPP method is trying to compensate for idiotic users, which shouldn't be our problem. I almost like the solution I just proposed (the just-in-case include) more. Why inundate our users with errors when we don't need to? Their C++ style is their own problem, and it's definitely common knowledge that you shouldn't be including the _impl.hpp files.

One more thing:

It seems entirely correct for our users to #include and nothing else.

Well, in a large library like ours that isn't really the right thing to do. Instead I would expect a user to write #include <mlpack/methods/neighbor_search/neighbor_search.h> and only include what they need.

Admittedly, we haven't the time right now to discover exactly which armadillo dependencies we're using (though likely we aren't using very many of them)

Modifying our core.h to only include parts of Armadillo is a bad idea. A user including MLPACK methods is able to assume that Armadillo is included, and if some methods aren't available because not everything is included, things break. Yes, we may lose compile time as a result, but really this is a can of worms we should not open.

[rcurtin]

Commented by jcline3 on 26 Jul 41836061 04:52 UTC

Well, in a large library like ours that isn't really the right thing to do. Instead I would expect a user to write #include <mlpack/methods/neighbor_search/neighbor_search.h> and only include what they need.

I don't see the point in that, since any method they #include will #include core.h, and thus everything anyway.

[rcurtin]

Commented by mamidon on 27 Dec 41836432 22:27 UTC
Replying to jcline3:

Well, in a large library like ours that isn't really the right thing to do. Instead I would expect a user to write #include <mlpack/methods/neighbor_search/neighbor_search.h> and only include what they need.

I don't see the point in that, since any method they #include will #include core.h, and thus everything anyway.
I think he's referring to the fact that including more than is necessary is wasteful, and may slow down compile times, especially on larger projects.

[rcurtin]

Commented by jcline3 on 6 Nov 41836592 03:21 UTC

I think he's referring to the fact that including more than is necessary is wasteful, and may slow down compile times, especially on larger projects.

You're right. Somehow I got it into my head that core.h includes all the methods as well.

[rcurtin]

Commented by rcurtin on 5 Feb 41954245 16:07 UTC
The solution upon which we agreed (the 'in case it hasn't already been included, include it') solution is now used in the entire project.