On Fri, Apr 11, 2014 at 08:48:02PM -0700, Ahmed Charles wrote:
The review of Boost.Align by Glen Fernandes commences today, Friday, 11 April, and concludes Sunday, 20 April. Please always state in your review whether you think the library should be accepted as a Boost library.
I believe that the Align library should be accepted into Boost without any conditional requirements from my side.
Additionally, please consider giving feedback on the following general topics:
- What is your evaluation of the design?
The aligned allocation/destruction functions are similiar in shape to the platform specific functions with similiar names. Judging by the documentation, the allocators and functions have as natural interfaces as possible, which is good.
- What is your evaluation of the implementation?
I've skimmed the implementation and no apparent madness stands out.
- What is your evaluation of the documentation?
Documentation as it currently stands needs some Boostification, but I believe that has been addressed already. I find the standardese-like reference readable.
- What is your evaluation of the potential usefulness of the library?
This library is very useful to anyone that ever has needed to deal with alignment, saving them from adapting and reinventing the facilities present in the library.
- Did you try to use the library? With what compiler? Did you have any problems?
I ran the bundled tests with a set of compilers with no additional flags, with Align implanted into a modular Boost checkout. Works perfectly: GCC 4.6.3, Intel 13.0, Intel 13.1 Compiler errors: PGI 13.10, PGI 14.3 (needed to modify Boost.Build to avoid linker failures) A lot of ambiguities like "aligned_allocator_adaptor_test.cpp", line 31: error: "allocator" is ambiguous Intel 12.0 Breaks in Boost.Config on __int128, so not really Align's fault. Runtime errors: PSC 5, may be due to site customizations of the toolchain so may be disregarded, I have to look more into it. is_aligned_test.cpp fails intermittently for sizes 128, 64, and sometimes 32. is_aligned_test.cpp(44): test 'boost::alignment::is_aligned(32, &s[0])' failed in function 'int main()' is_aligned_test.cpp(49): test 'boost::alignment::is_aligned(64, &s[0])' failed in function 'int main()' is_aligned_test.cpp(54): test 'boost::alignment::is_aligned(128, &s[0])' failed in function 'int main()' Fails 'align_test.cpp' assertions on lines 23, 25, 31, 32, 33, 40, 42; once for 64 and once for 128 bytes. Sadly SunStudio and VisualAge are out of my reach at the moment, checking out modular Boost on those filesystems would take days. I can provide logs on request.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
An evening skimming and reflecting on the documentation and intended semantics, a morning running the test suites on any compilers I could reach.
- Are you knowledgeable about the problem domain?
I've reinvented most facilities of this library in more or less horrible ways in the past. -- Lars Viklund | zao@acc.umu.se