[mlpack-svn] [MLPACK] #155: order of includes and include protections

MLPACK Trac trac at coffeetalk-1.cc.gatech.edu
Tue Nov 1 01:08:15 EDT 2011


#155: order of includes and include protections
--------------------------------------------------+-------------------------
  Reporter:  nslagle                              |        Owner:     
      Type:  wishlist                             |       Status:  new
  Priority:  major                                |    Milestone:     
 Component:  MLPACK                               |   Resolution:     
  Keywords:  includes, templates, style, nitpick  |     Blocking:     
Blocked By:                                       |  
--------------------------------------------------+-------------------------

Comment (by rcurtin):

 > 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 <mlpack> 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.

-- 
Ticket URL: <http://trac.research.cc.gatech.edu/fastlab/ticket/155#comment:9>
MLPACK <www.fast-lab.org>
MLPACK is an intuitive, fast, and scalable C++ machine learning library developed by the FASTLAB at Georgia Tech under Dr. Alex Gray.


More information about the mlpack-svn mailing list