Giovanni, thanks for your review. Giovanni Piero Deretta wrote:
On Dec 3, 2007 10:57 AM, John Torjo <john.groups@torjo.com> wrote:
Hi all,
The formal review of the Boost.Functional/Forward library, proposed by Tobias Schwinger, begins today :
* What is your evaluation of the design?
Good. Simple enough to cover most needs.
In my design I allow the user to specify if a specific operator() overload should be disabled or not (depending on the arity and the parameter types); It is useful if you are using more than one forwarder in some complex composition (i.e. as base classes) and you need to disable ambiguous overload. I do not think Boost.Functional/Forward should necessarily provide this functionality, but it might be considered.
Yes, we have been there (with Fusion) and recently dropped this functionality when porting to Boost.ResultOf: We used to have an empty 'result' metafunction say "disable this overload", exploiting SFINAE where applicable. ResultOf, however, does not propagate emptiness by specification as a matter of downward compatibility (no SFINAE inside the operand of 'decltype'). I personally still have mixed emotions about the loss.
I think too that the library should optionally allow K more arguments which are all const or non const. For (uncommon) very high arity functions that need to be wrapped, you can usually live with this limitation.
* What is your evaluation of the implementation?
Looks simple, but it is hard to evaluate preprocessor meta code.
* What is your evaluation of the documentation?
Complete enough.
I think it should be explicitly specified that the wrapped function object must be result_of compatible (if such a note is already present, I've missed it).
OK, here is some overlap with Joel's complaints. It seems I should add some detail in regard to result_of usage.
A discussion of the impact of the library on compile time (especially with high values of N) would be useful.
Yes, I think the final version will have a it and give the user more control over the N (see my reply to Joel's review).
Also a comment on the (potentially lack of) runtime performance penalty imposed by the library would be nice.
That would be "the lack thereof with a decent compiler". That is there are no constructs that force the compiler to emit code - though it's still free to do so.
From the documentation I cannot infer if operator() is overloaded for both const and non const 'this'. I think not (), but it should be specified explicitly.
Count it as a bug.
I have never needed this in practice, In fact, IMHO it should be specified that 'this' is always const.
Considerable, but I believe I want to think some more about it :-). Regards, Tobias