Re: [boost] [Fit] Formal review / Kris

Subject: Re: [boost] [Fit] Formal review / Kris
From: paul Fultz (pfultz2_at_[hidden])
Date: 2016-03-19 20:45:45

> On Saturday, March 19, 2016 2:26 PM, Krzysztof Jusiak <krzysztof_at_[hidden]> wrote:
> > Hi,
> 
> Firstly, sorry for submitting my review so late.
> 
>>  - Whether you believe the library should be accepted into Boost
> 
> Yes. Although the submission is not perfect, I prefer boolean logic, and,
> therefore Fit a 1 for me.
> 
>>  - Your name
> 
> Krzysztof Jusiak
> 
>>  - Your knowledge of the problem domain.
> 
> I have been implementing similar solutions myself when experimenting
> with lambdas. I have also checked out functional part of Hana library.
> 
>>  You are strongly encouraged to also provide additional information:
>>  - What is your evaluation of the library's:
>>     * Design
> 
> IMHO design is decent. Functionality is loosely coupled and really easy to
> follow.
> There are a few inconsistencies here and there but there were pointed out
> already
> and they don't have a huge impact on the overall solution.
> 
>>     * Implementation
> 
> Quite a lot of macros but most of these are due to lack of language
> features or compilers bugs.
> Besides that, it looks alright. Moreover, I do agree with most issues
> pointed out already.
> 
>>     * Documentation
> 
> Motivation could have been much better. Examples showing how much pain might
> be saved using Fit in comparison to writing the same things by hand would
> be great.
> Moreover, ready to copy examples would simplify testing the library.
> Personally, I would also add disassembled code for these examples showing
> how
> well-optimized code Fit can produce. Maybe, it's worth to add try-it-online

> functionality too?

Thats a good idea. I'll try show some disassembly to show how well the
optimizer does. I like the idea of having a try-it-online, however, I would
like a way to better handle versioning and offline documentation. I am sure
there is a nice solution to this I just haven't time to figure it out yet.

> 
>>     * Tests
> 
> Tests are short and easy to follow. I can't see coverage but it seems like
> most
> things are tested. Travis addition with each push make me confident that
> the library
> is always in a good shape.
> I would like to see performance tests though for both compilation times and

> run times.

Yes, I have run a couple of local tests to test performance. I could pull
together some report to put in the documentation. Ideally. it would be good to
have a comparison with other libraries as well. I haven't compared it with
Hana yet, either, which would be a good idea. However, I do know that the
implementation of `fit::conditional` is much faster than the implemenation
used for `overload_linearly` in Hana, but I haven't done a direct comparison.
I'll try to add something like that. I think it will be useful.
> 
>>     * Usefulness
> 
> IMHO Fit is defo a library to have in Boost for modern C++ development.
> I find the solutions provided by Fit very useful as I have been writing
> similar functionality myself. Nevertheless, documentation needs improving
> (as it was stated already) in order to state the purpose more clearly with
> clean examples. I have been worrying that there will be a clash with Hana.
> Having 2 modern libraries providing similar functionality would be
> confusing,
> however, my concerns were already clarified by Louis.
> 
>>  - Did you attempt to use the library? If so:
>>     * Which compiler(s)
> 
> Clang 3.7/3.8/HEAD, MSVC 2015
> 
>>     * What was the experience? Any problems?
> 
> No issues.
> 
>>  - How much effort did you put into your evaluation of the review?
> 
> A few hours for the review, however, I have been looking and trying

> Fit for a while.

Thanks Kris for the review and feedback.

> 
> Cheers,
> Kris
> 
> _______________________________________________
> Unsubscribe & other changes: 
> http://lists.boost.org/mailman/listinfo.cgi/boost
>