On 02/18/2012 03:08 PM, Joel Falcou wrote:
================ Writing a Review ================ The reviews and all comments should be submitted to the developers list, and the email should have "[PREDEF] Review" at the beginning of the subject line to make sure it's not missed. Please explicitly state in your review whether the library should be accepted.
I do not think that this library should be accepted in Boost, and I think a fairly important overhaul would be required to get it to a satisfying state.
The general review checklist:
- What is your evaluation of the design?
Checking for different compilers in independent files simply doesn't work due to some compilers emulating others. We have talked about __GNUC__, but there are several compilers that define _MSC_VER as well. The library should use, like Boost.Config and Boost.Detect, a chain if/elif to ensure that all values are mutually exclusive. Other issues in the design include the way macros are defined even if the associated element is not detected. This is fairly confusing for the user. The design is not sufficiently good to make Boost.Config rely on it.
- What is your evaluation of the implementation?
Implementation does not do what is expected, as was demonstrated by Artyom's tests. It detects x86-64 as Itanium, wrongly detects the standard library (detects two of them at the same time, while only one should be possible), detects clang as gcc, etc. Not robust enough to become the most basic piece of the Boost infrastructure, being included with every other Boost header.
- What is your evaluation of the documentation?
Documentation was good, but misled me to believe the library didn't do things as it does. It would probably be a good idea to say in what conditions macros are defined. The documentation made me assume all macros were mutually-exclusive: clearly this is not the case at all, which is why this library is not suitable for inclusion.
- What is your evaluation of the potential usefulness of the library?
This library, if done right, would be very useful to correctly deal with frontend-specific, backend-specific, library-implementation-specific, architecture-specific or OS-specific code. I therefore strongly encourage people to submit more proposals on the subject.
- Did you try to use the library? With what compiler? Did you have any problems?
I did not try to use the library. I read the documentation and skimmed through the code.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
A quick reading.
- Are you knowledgeable about the problem domain?
Yes.
And finally, every review should answer this question:
- Do you think the library should be accepted as a Boost library?
No.