Re: [boost] [PREDEF] Review for the Boost.Predef library by Rene Riviera

Subject: Re: [boost] [PREDEF] Review for the Boost.Predef library by Rene Riviera
From: Artyom Beilis (artyomtnk_at_[hidden])
Date: 2012-02-20 03:06:47

Hello,

This is my review:


>    - What is your evaluation of the design?

I personally do not like it for two reasons:

When you include a header like <boost/predef.h>
you get 103 files included... 103 stats, fopen, fclose,
read parse and so on.

More then that, consider build systems that should stat 103 files
for each cpp file compiled.

I know that this library created as "header only" but Boost has
enough header-only configurations and not a single normal compile
time configuration.

I assume that better configuration can be created using compilation
rather then explicit defines test, also it would be much more expendable
for future directions.

I think it would be great if a single file like <boost/conf_predef.h>
would include all definitions. Something that can be generated
at build time and it would include all needed defines making 

the "include" much faster and easier to read.

Also I'd merge multiple files like boost/prefef/os/* to a single
one to improve compilation times.

Also generating compiled time header would allow to to extend the
macros to new definitions that can't be done today for example:

- BOOST_POINTER_SIZE  4 or 8 (or even 2)
- BOOST_WCHAR_T_SIZE 2 or 4

And so on.

Basically two things I'd like to see improved:

1. Do not create 103 files but several maybe bigger files.

2. Add compile time configuration generation for stuff
   that can't be generated using headers only.


>    - What is your evaluation of the implementation?

I think some stuff can be more fine grained.
--------------------------------------------


For example OS Version for
Windows... There are macros like _WIN32_WINNT (AFAIR) that gives you
quite nice OS version.

Another thing is for example libstdc++ version. I'd rather expect to
have for example SO version like 6.x - similar to libstdc++.so.6.x
It can be configured by mapping year -> version.

Or use compiler version - library binded to specific compiler.


It is not really intuitive because I do not remember in what year

gcc-4.5 was released but I do know when for example unique_ptr was
introduced.

In this case I think it can be derived from gcc version and the
fact that libstc++ library is used.


Other things like for example LANGUAGE_STDCPP... We already have
__cplusplus that does almost exactly the same as this macro. Why
do we need it and what added value it gives us?


Architecture
------------

I notices several problems:

1. Arm has several architectures ARM, ARMEL should be distinguished
2. There are MIPS and MIPSEL - Little Endian MIPS (debian has both for example)

I'd suggest to check carefully all acrchitecures.

Compiler:
---------

There is no "Cygwin" compiler or "MINGW" compiler, it is GCC. Should be fixed

OS:
---

- I'd rather like to See Darwin then MACOS
- MACOS not good.

  It should be MACOSCLASSIC and MACOS or even better:

  MACOS and DARWIN

  Mac OS 9 and Mac OS 10 share amlost nothing, on the other hand
  Darwin and Mac OX 10 is almost the same.

Testing...
---------

1. Unfortunately I see none on this. I understand that is is not simple but
   you may probably use some tools like `uname` and compare their output
   any maybe use for example bjam toolset and bjam os and compare it with detected
   compiler.

   From what I can see all tests are manual. It is not good.

2. I'd like to read how did you tested this project... I don't see any of it
   in the documentation.

   Have you setup at least several VMs using for example qemu with different
   architectures? Have to tested this on several OSs?




>    - What is your evaluation of the documentation?

It needs to be improved, for example what BOOST_LIBSTD_CXX version means?

Does BOOST_OS_WINDOWS defined when CYGWIN is used


Only from looking at the source I could figure this out.
I think each macro should display exact meaning for user who is interested.


>    - What is your evaluation of the potential usefulness of the library?

It should be very useful. Because it standardizes different macros
and makes it easier to figure out for example how to check if we are
using Solaris



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

Yes, I did and I had some problems.

1. Almost all tests do not even compile on Cygwin...
2. Under mingw gcc-4.5.3 I see following output of info_as_cpp

   For example:

   ** Detected **
   ....
   BOOST_LIBSTD_GNU = 410400028 (41,4,28) | GNU
   ...

   How should I treat it?


However I mostly read the code and the documentation.


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

2-3 hours of code and documentation reading + writing this e-mail.


>    - Are you knowledgeable about the problem domain?
>

Yes, I needed such macros many times and created such configurations.



>And finally, every review should answer this question:
>
>    - Do you think the library should be accepted as a Boost library?
>
>Be sure to say this explicitly so that your other comments don't obscure your overall opinion.
>
>


Unfortunately, No.

But I strongly recommend to improve the library and submit it for further review.

Problems:

1. Too many macros are not fine grained as they should be
   or they generate unexpected or surprising definitions.

2. Documentation must be improved because for now it is very unclear.
3. Many problems with different definitions like there is no such
   thing like Cygwin compiler should be fixed.
4. 103 files in headers that would slow down the build process.
5. No real test suite exists

So general direction is good but the library is far from being
production ready.


Artyom Beilis
--------------
CppCMS - C++ Web Framework:   http://cppcms.com/
CppDB - C++ SQL Connectivity: http://cppcms.com/sql/cppdb/