[boost] [PREDEF] Review

Subject: [boost] [PREDEF] Review
From: Dan Price (dkprice_at_[hidden])
Date: 2012-02-29 17:26:43

Hi everyone,

This is my first review of a boost library but I have been a long-time
boost user. I am excited by this proposal because it is a library that
I would use on the day that it is accepted and it fills the need for
what is otherwise a hackish and inconsistent task. My review of the
library is included below.

Thank you,

Dan Price

**What is your evaluation of the design?**

The macros are clear. The BOOST_ARCHITECTURE_ macros are a little too
long to type; I would advocate for changing them to BOOST_ARCH_ to
make it quicker to type. Note that the operating system macro is
BOOST_OS_ and not BOOST_OPERATINGSYSTEM, and BOOST_CXX vs.
BOOST_COMPILER, so there is already a precedent for shorter names. The
other ones are easy to type and appropriate.

Within the headers there are human-readable strings that are intended
to be used with a built-in test. It seems as if these might be useful
to make these available for other purposes such as debugging.

I wanted to add a comment to a question that showed up prevalently on
the mailing lists regarding NULL vs. zero for definitions. Before
reading this thread, but after having read the documentation, I
assumed that definitions took on a NULL (undef) value. That is to say
that even after reading the documentation, I made a faulty assumption
because of my previous experience with platform detection. I would
therefore advocate that an #undef approach be used rather than the
zero-based one in order to avoid any gotchas.

**What is your evaluation of the implementation?**

The design goals of minimal "weight" from the predef headers is a good
one. I personally do not think there is much to worry about regarding
the implementation as long as the user-facing features (names,
completeness) are good.

**What is your evaluation of the potential usefulness of the library?**

This is a library that I would use on the day that it was introduced
to boost and I would even consider patching the distribution of boost
I have in version control with this library.

**Did you try to use the library?  With what compiler?  Did you have
any problems?**

I compiled the tests on a 64-bit version of ubuntu Linux and they
worked as expected.

**How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?**

My effort consisted of reading through the documentation and exploring
the headers. I also compiled the tests on one platform and they worked
as expected.

**What is your evaluation of the documentation?**

The documentation is acceptable, but I think that version number
description can be cleaned up a bit. Specifically, rather than
repeating "Version number available as major, minor, and patch." over
and over, perhaps a graphic or an abbreviation can be used?
Alternatively this may be better laid out as a table.

One error in the documentation (also in the source): "American Micro
Devices" -> "Advanced Micro Devices"

**Are you knowledgeable about the problem domain?**

I am knowledgeable of the preprocessor symbols of the 3 compilers and
3 systems I typically use. My normal habit for systems I am unfamiliar
with is to search for information at predef.sourceforge.net but this
library would clean up and simplify that work considerably.
Additionally, I can see my team members who are not as familiar with
detection mechanisms finding this library very useful.

**Summary**

In summary, I think that this library should be *conditionally
accepted* as a boost library, provided that the issue regarding the
NULL vs. 0 values for preprocessor symbols be worked out. I am an
advocate of the NULL-based approach because it best matches what my
expectations are.