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: Rene Rivera (grafikrobot_at_[hidden])
Date: 2012-02-21 02:17:29

On 2/20/2012 2:06 AM, Artyom Beilis wrote:
> This is my review:

Thank you for taking the time to do a review. Much appreciated!

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

That thought did cross my mind when deciding on the header structure. 
But, given how many compilers support some form of precompiled inclusion 
I decided that it was OK to go with the modular arrangement. Especially 
for the much easier maintainability of the library. One option I can 
think of is to provide a single concatenated header that is generated 
from the current modular headers. This, of course is a bit more work on 
the library side, and is considerably harder for users to humanly 
understand one big concatenated header file. Hence this is something 
that I would seriously consider adjusting if the number of headers 
becomes a real measurable problem. Which also suggests that I should add 
some header parsing performance tests.

> 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 do mention in the future tasks for the library that having some form 
of pre-compile external configuration as a way to add configuration for 
external libraries would be a good idea. But the scope, and hence goals, 
of the current set of definitions is limited to preprocessor defined 
symbols.

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

For just all the definitions that this library currently does I can't 
see how a compiled test could do any better. Since the compile test 
would do exactly what the library currently does as the definitions the 
library is based on are equivalent to what you could discover outside of 
the preprocessor.

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

I disagree that this would make it easier to read. If one has to 
generate multiple "conf_predef.h" files, because one is dealing with 
multiple configurations, then it becomes less readable as one is trying 
to keep in your head, and manage with the build system, multiple 
versions of the same file. I've been there with auto-conf libraries, and 
written build system management for this, and it's a management pain to 
juggle the multi-conf design (from a user point of view).

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

I mentioned above how I would consider doing that. Of course the 
drawback is that then one ends up with a large single file that one has 
to search around in to find what you may be looking for. Hence making 
documentation paramount :-)

> 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

There is a fine line between the current goal of this library vs. what 
the goals of the Boost Config library. Currently this library is not 
about defining "features" as that is what Boost Config does. I'm not 
objecting to having the above definitions.. Just need to point out that..

> And so on.

The "so on" would be subject to the goals ;-)

> Basically two things I'd like to see improved:
>
> 1. Do not create 103 files but several maybe bigger files.

If I'm going to do that, I'd create just one file. After all if file 
system performance is the reason for doing it then it makes more sense 
to eliminate as much of it as possible.

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

Yes, as mentioned in the "todo" section.

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

It certainly would be nice to deal with all the complexity of the 
Windows version definitions. I'll add that to some future release.

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

I shied away from doing that as it then becomes a continual maintenance 
task. It especially means that users will be put in a situation where 
the predef library will lag behind in having mapped version definitions. 
Of course, ideally libstd++ would supply such a mapped version number 
directly and I could use that. But I failed to find such a definition.

> Or use compiler version - library binded to specific compiler.

That is not sufficient. You can have a compiler that can target multiple 
configurations and hence this version may not have any relevance to the 
library one is actually compiling to. Unless you meant something else?

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

Actually you'd have to look up more than just the year. You would need 
the exact date to be sure you have to correct condition as any 
particular feature may have been added late in a year where there are 
multiple releases as has been the case for all gcc release 
<http://gcc.gnu.org/releases.html>.

> 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?

I think the same can be argued for all the definitions that this library 
provides. It gives us the ability to check for the version, or just 
presence, with a *single* *consistent* *readable* interface.

> Architecture
> ------------
>
> I notices several problems:
>
> 1. Arm has several architectures ARM, ARMEL should be distinguished

Sorry if I don't know.. But what is ARMEL is exactly in relation to ARM? 
In particular how does it relate to the various ARM chips produced 
<http://en.wikipedia.org/wiki/ARM_architecture>? And how would I 
determine an ARMEL?

> 2. There are MIPS and MIPSEL - Little Endian MIPS (debian has both for example)

Same question as above <http://en.wikipedia.org/wiki/MIPS_architecture>. 
Hm.. Or is "EL" some tag for specifying the target endian-ness? Because 
if it is, adding an orthogonal endian definition is something I'd like 
to do in the near future. Although, like the "word" size, is not 
something as easily done as just enumerating architectures.

> I'd suggest to check carefully all acrchitecures.

That's always the case for all the predefs ;-)

> Compiler:
> ---------
>
> There is no "Cygwin" compiler or "MINGW" compiler, it is GCC. Should be fixed

Right. My mistake there. Makes more sense to test for OS_CYGWIN and 
OS_WINDOWS.

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

Well given recent development it might make more sense to just use MACOS 
and OSX. But yes, I'll consider better naming for those. Although given 
that the current definitions only cover 9.0 vs 10.0, it might not yield 
much of a gain in clarity.

> Testing...
> ---------
>
> 1. Unfortunately I see none on this.

There is testing for at least the functionality of the version macro and 
the decomposition utility macros. So I object to the "none" 
characterization.

Testing for this library was something I asked about previously on this 
list. And there were not any really attainable solutions. The solutions 
boiled down to either simulate the target configuration or human 
verification. Unfortunately the former is essentially a tautological 
test as the test is specifying both the definition and test. Human 
verification has the benefits that it is distributed and in this use 
case doesn't need to be repeated.

> I understand that is is not simple but

Indeed :-\

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

No I can't. Any external mechanism would almost certainly test the host 
configuration not the target configuration. And the commonplace 
cross-compilation world we now live in this is not something that can be 
dismissed. Hence the only likely external test would involve running the 
compilers themselves and those would require prior knowledge of what you 
are targeting. And, consequently, likely running into the tautological 
test problem.

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

Not all tests.

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

I could certainly add such documentation. Although it would be 
continually expanding.

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

The current testing only involved the automated testing I mention above 
for the version and decomposition macros. Plus testing in the platforms 
that I have immediate access to: Windows, OSX, Linux (OpenSUSE), and iOS.

>>      - What is your evaluation of the documentation?
>
> It needs to be improved, for example what BOOST_LIBSTD_CXX version means?

I do point to the libraries web site <http://libcxx.llvm.org/>. Is that 
not enough to answer what it is? And the macros does say "If available 
version number as major, minor, and patch". Although now that I reread 
the definition it's actually only version and patch that are available.

> Does BOOST_OS_WINDOWS defined when CYGWIN is used

I'll add a note about that when I find out if gcc on cygwin defines the 
windows macro.

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

Hm, exact meaning would boil down to including the source in the 
documentation. But it would be worth it to list which predefined macros 
each macro uses in it's implementation. That should cover most cases 
without making the documentation just parrot the source.

> 1. Almost all tests do not even compile on Cygwin...

Could you elaborate? What cygwin version are you trying?

> 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?

The docs say "Version number available as year (from 1970), month, and 
day." I.e. the above says libstdc++ released on 2011/04/28.

>>      - Do you think the library should be accepted as a Boost library?
>
> Unfortunately, No.

Sorry to hear that..

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

Do you have more examples than the ones you mentioned above?

> 2. Documentation must be improved because for now it is very unclear.

All documentation can always be improved ;-) Did my answers above 
address this for you?

> 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

Is there more I can expand on to address those?

-- 
-- Grafik - Don't Assume Anything
-- Redshift Software, Inc. - http://redshift-software.com
-- rrivera/acm.org (msn) - grafik/redshift-software.com
-- 102708583/icq - grafikrobot/aim,yahoo,skype,efnet,gmail