-
Notifications
You must be signed in to change notification settings - Fork 34
Description
I've started writing a Conan recipe in order to cleanly integrate wide-integer into CNL. The plumbing and the conclusions I've come up with are hopefully applicable beyond CNL's concerns so I though I'd share the following observations/suggestions:
- There are two public include directories (the thing that GCC receives as a header search path with options like
-Ior-isystem):- ./math/wide-integer/, and
- ./util/utility/.
- There are two public global namespaces:
util, andmath/wide_integer.
- Two of the four headers exposed in public include directories are only necessary for testing, not integration with dependent packages:
- uintwide_t_examples.h, and
- uintwide_t_test.h.
In the CMake or Conan scripts, it is possible to shuffle some of the files around in order to omit the test headers from the install destination. However, because of this line
#include <util/utility/util_dynamic_array.h>
two separate install directories are needed and neither -- at their root -- do very much to indicate that they contain headers specific to the wide-integer library. The namespaces suffer from a similar problem: they are one or two levels deep and given very open-ended names at their root: 'util' and 'math'.
Like namespaces, directories (under usr/include at least) are not for creating taxonomies. (That's a great video to watch generally but that nugget of advice translates very well to C++.) By using 'util' and 'math', you're more likely to risk collisions and it's less likely for users to be able to find and remember where you library is on their system. So please consider some minor rearrangement to your source files and their location -- even if you don't go with the following suggestions...
My recommended changes (which are just one possible solution and which I'd be happy to submit in a PR) are to:
- move test-only headers out of the public search path;
- decide on a namespace and a directory for the library (e.g.
::wide_integerand /wide-integer/); - move public library headers together and away from the rest of the project files, e.g.:
- ./include/wide-integer/uintwide_t.h, and
- ./include/wide-integer/dynamic_array.h;
- move the public definitions into this top-level namespace, e.g.
::wide_integer::uintwide_t; - put non-public (but header-exposed) definitions in a
detailsub-namespace, e.g.::wide_integer::detail; - move
util::dynamic_arrayand its comparison operators to thedetailsub-namespace.
I suggest this with virtually no understanding of:
- the other libraries that might share
dynamic_array(there are ways to keep it hidden indetailfor wide-integer but inject it into another, public namespace elsewhere), and - the users of wide-integer who would be disrupted by this change. A less disruptive change is entirely possible but you may find that they appreciate the added clarity -- especially if it is accompanied by build and package management facilities which are simple and idiomatic.