[Review:Contract]
Hi, first of all, YES, the library should be included in BOOST, in my opinion. In addition: * What is your evaluation of the design? Very cool. Looks like a 1:1 translations from some good algorithm books into code :-) * What is your evaluation of the implementation? I spent some time browsing through the macro code hoping it would help me to solve a problem of my own which looked similar (turned out it wasn't). Never being a fan of too much macro code, I have to say that this is very nicely written! My only concerns: - It seems to me that the implementation is spread over more files than necessary. It made following the call stack a bit tiresome. - I a bit torn between a) Steven's comment who would like to see CONTRACT_OLDOF being replaced by the would-be keyword in a possible future standard b) boost macros being clearly visible as such, leaving translation to future standard keywords to the user of the library, e.g. #define foreach BOOST_FOREACH I tend towards b). * What is your evaluation of the documentation? Excellent! * What is your evaluation of the potential usefulness of the library? It is a very good proof of concept and hopefully the basis for continued discussions about including contracts into the C++ language. Due to a lack of formal contracts in much of day to day programming and the enormous compile time overhead, I don't see the library being used all over the place anytime soon, but I expect it to be used in generic algorithms, helping both developers and library users. * Did you try to use the library? With what compiler? Did you have any problems? Sadly: No. Did not find the time. * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I spent a few hours with the code, maybe two hours with the documentation and I followed some of the discussions during the review period. * Are you knowledgeable about the problem domain? I would not call myself an expert, but I know enough to say: I'd really like to have Boost.Contract available the next time I write a mission-critical function/class/library! Best regards, Roland PS: Sorry for being late. I'm hoping for lenience since the review period was announced a bit late as well :-)
On Sun, Sep 2, 2012 at 2:28 AM, Roland Bock <rbock@eudoxos.de> wrote:
Hi,
first of all, YES, the library should be included in BOOST, in my opinion.
Thanks for submitting the review.
In addition:
* What is your evaluation of the design? Very cool. Looks like a 1:1 translations from some good algorithm books into code :-)
* What is your evaluation of the implementation? I spent some time browsing through the macro code hoping it would help me to solve a problem of my own which looked similar (turned out it wasn't).
What's the problem you are trying to solve? (Maybe post it in a separate email thread.)
Never being a fan of too much macro code, I have to say that this is very nicely written!
My only concerns: - It seems to me that the implementation is spread over more files than necessary. It made following the call stack a bit tiresome.
I refactored the files many times... what's there now makes sense to me, modularity is important given the amount of pp code this lib has. Do you have any specific suggestion on some files that should be merged together?
- I a bit torn between a) Steven's comment who would like to see CONTRACT_OLDOF being replaced by the would-be keyword in a possible future standard b) boost macros being clearly visible as such, leaving translation to future standard keywords to the user of the library, e.g. #define foreach BOOST_FOREACH
I tend towards b).
I just replied to Steven's. CONTRACT_OLDOF is a "special" macro so #define oldof CONTRACT_OLDOF won't work (because you can't guarantee proper expansion order that way) but I can provide a CONTRACT_CONFIG_DEFINE_OLDOF_KEYWORD that will #define oldof for you (even if oldof doesn't follow the Boost macro naming standard).
* What is your evaluation of the documentation? Excellent!
* What is your evaluation of the potential usefulness of the library? It is a very good proof of concept and hopefully the basis for continued discussions about including contracts into the C++ language.
Due to a lack of formal contracts in much of day to day programming and the enormous compile time overhead, I don't see the library being used all over the place anytime soon, but I expect it to be used in generic algorithms, helping both developers and library users.
* Did you try to use the library? With what compiler? Did you have any problems? Sadly: No. Did not find the time.
* How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I spent a few hours with the code, maybe two hours with the documentation and I followed some of the discussions during the review period.
* Are you knowledgeable about the problem domain? I would not call myself an expert, but I know enough to say: I'd really like to have Boost.Contract available the next time I write a mission-critical function/class/library!
Thanks. --Lorenzo
* What is your evaluation of the implementation? I spent some time browsing through the macro code hoping it would help me to solve a problem of my own which looked similar (turned out it wasn't). What's the problem you are trying to solve? (Maybe post it in a separate email thread.) Will do, topic will be "Enum Conversion".
Never being a fan of too much macro code, I have to say that this is very nicely written!
My only concerns: - It seems to me that the implementation is spread over more files than necessary. It made following the call stack a bit tiresome. I refactored the files many times... what's there now makes sense to me, modularity is important given the amount of pp code this lib has. Do you have any specific suggestion on some files that should be merged together? No specific suggestions, I just tried to follow the "requires" through
- I a bit torn between a) Steven's comment who would like to see CONTRACT_OLDOF being replaced by the would-be keyword in a possible future standard b) boost macros being clearly visible as such, leaving translation to future standard keywords to the user of the library, e.g. #define foreach BOOST_FOREACH
I tend towards b). I just replied to Steven's. CONTRACT_OLDOF is a "special" macro so #define oldof CONTRACT_OLDOF won't work (because you can't guarantee proper expansion order that way) but I can provide a CONTRACT_CONFIG_DEFINE_OLDOF_KEYWORD that will #define oldof for you (even if oldof doesn't follow the Boost macro naming standard). Hmm. I wonder if precondition, postcondition, requires etc. might have
On 2012-09-02 19:49, Lorenzo Caminiti wrote: the code and found myself hopping from file to file for every other function. At least it felt that way. the potential to confuse users who are not aware of Boost.Contract? They look like language keywords, but are macros instead. That's why I said I am tending towards b) which would at least define the keyword locally, e.g. #define precondition BOOST_CONTRACT_PRECONDITION If that is not possible (as with oldof), I could still write: CONTRACT_CONFIG_DEFINE_OLDOF_KEYWORD // defines oldof Regards, Roland
On Sun, Sep 2, 2012 at 12:13 PM, Roland Bock <rbock@eudoxos.de> wrote:
* What is your evaluation of the implementation? I spent some time browsing through the macro code hoping it would help me to solve a problem of my own which looked similar (turned out it wasn't). What's the problem you are trying to solve? (Maybe post it in a separate email thread.) Will do, topic will be "Enum Conversion".
Never being a fan of too much macro code, I have to say that this is very nicely written!
My only concerns: - It seems to me that the implementation is spread over more files than necessary. It made following the call stack a bit tiresome. I refactored the files many times... what's there now makes sense to me, modularity is important given the amount of pp code this lib has. Do you have any specific suggestion on some files that should be merged together? No specific suggestions, I just tried to follow the "requires" through
- I a bit torn between a) Steven's comment who would like to see CONTRACT_OLDOF being replaced by the would-be keyword in a possible future standard b) boost macros being clearly visible as such, leaving translation to future standard keywords to the user of the library, e.g. #define foreach BOOST_FOREACH
I tend towards b). I just replied to Steven's. CONTRACT_OLDOF is a "special" macro so #define oldof CONTRACT_OLDOF won't work (because you can't guarantee proper expansion order that way) but I can provide a CONTRACT_CONFIG_DEFINE_OLDOF_KEYWORD that will #define oldof for you (even if oldof doesn't follow the Boost macro naming standard). Hmm. I wonder if precondition, postcondition, requires etc. might have
On 2012-09-02 19:49, Lorenzo Caminiti wrote: the code and found myself hopping from file to file for every other function. At least it felt that way. the potential to confuse users who are not aware of Boost.Contract? They look like language keywords, but are macros instead. That's why I said I am tending towards b) which would at least define the keyword locally, e.g.
#define precondition BOOST_CONTRACT_PRECONDITION
No, precondition, postcondition, requires, intialize, extends, etc are not macros, only CONTRACT_OLDOF is a macro. The others are indentifiers that have special meaning when used in specified places within the library macros. That's all. In a way these are similar to C++11 virtual specifiers (which are also not keywords) but for the library instead of the language. For example, you could do: #include <contract.hpp> CONTRACT_FUNCTION( void (precondition) ( int x ) precondition( x >= 0 ) ) { } int main ( void ) { precondition(0); return 0; } I advice not to use precondition, etc outside the uses that the lib does. However, if you have existing code with a function, variable, etc named precondition, my lib will leave that alone (as it should). That won't be the case instead if we did #define oldof ... in which case if you have existing code that uses oldof it will be messed up. That said, I can't use the same pp parsing mechanism I use for precondition, etc also for oldof because oldof is nested within the assignment expression (while precondition, etc appear at the beginning of their syntactic element). precondition is the first token so it can be parsed as a specifier: precondition( ... ) // ok odlof is not the first (or last) token (and all other tokens around are arbitrary plus they can contain non-alphanumeric symbols) so it can't be parsed by the pp: auto old_size = oldof size() // nope :( but I can use a macro to get around this: auto old_size = CONTRACT_OLDOF size() // ok
If that is not possible (as with oldof), I could still write:
CONTRACT_CONFIG_DEFINE_OLDOF_KEYWORD // defines oldof
Thanks, --Lorenzo
On 2012-09-02 21:51, Lorenzo Caminiti wrote:
* What is your evaluation of the implementation? I spent some time browsing through the macro code hoping it would help me to solve a problem of my own which looked similar (turned out it wasn't). What's the problem you are trying to solve? (Maybe post it in a separate email thread.) Will do, topic will be "Enum Conversion".
Never being a fan of too much macro code, I have to say that this is very nicely written!
My only concerns: - It seems to me that the implementation is spread over more files than necessary. It made following the call stack a bit tiresome. I refactored the files many times... what's there now makes sense to me, modularity is important given the amount of pp code this lib has. Do you have any specific suggestion on some files that should be merged together? No specific suggestions, I just tried to follow the "requires" through
- I a bit torn between a) Steven's comment who would like to see CONTRACT_OLDOF being replaced by the would-be keyword in a possible future standard b) boost macros being clearly visible as such, leaving translation to future standard keywords to the user of the library, e.g. #define foreach BOOST_FOREACH
I tend towards b). I just replied to Steven's. CONTRACT_OLDOF is a "special" macro so #define oldof CONTRACT_OLDOF won't work (because you can't guarantee proper expansion order that way) but I can provide a CONTRACT_CONFIG_DEFINE_OLDOF_KEYWORD that will #define oldof for you (even if oldof doesn't follow the Boost macro naming standard). Hmm. I wonder if precondition, postcondition, requires etc. might have
On 2012-09-02 19:49, Lorenzo Caminiti wrote: the code and found myself hopping from file to file for every other function. At least it felt that way. the potential to confuse users who are not aware of Boost.Contract? They look like language keywords, but are macros instead. That's why I said I am tending towards b) which would at least define the keyword locally, e.g.
#define precondition BOOST_CONTRACT_PRECONDITION No, precondition, postcondition, requires, intialize, extends, etc are not macros, only CONTRACT_OLDOF is a macro. The others are indentifiers that have special meaning when used in specified places within the library macros. That's all. In a way these are similar to C++11 virtual specifiers (which are also not keywords) but for the
On Sun, Sep 2, 2012 at 12:13 PM, Roland Bock <rbock@eudoxos.de> wrote: library instead of the language. For example, you could do:
#include <contract.hpp>
CONTRACT_FUNCTION( void (precondition) ( int x ) precondition( x >= 0 ) ) { }
int main ( void ) { precondition(0); return 0; }
I advice not to use precondition, etc outside the uses that the lib does. However, if you have existing code with a function, variable, etc named precondition, my lib will leave that alone (as it should). That won't be the case instead if we did #define oldof ... in which case if you have existing code that uses oldof it will be messed up.
That said, I can't use the same pp parsing mechanism I use for precondition, etc also for oldof because oldof is nested within the assignment expression (while precondition, etc appear at the beginning of their syntactic element). precondition is the first token so it can be parsed as a specifier:
precondition( ... ) // ok
odlof is not the first (or last) token (and all other tokens around are arbitrary plus they can contain non-alphanumeric symbols) so it can't be parsed by the pp:
auto old_size = oldof size() // nope :(
but I can use a macro to get around this:
auto old_size = CONTRACT_OLDOF size() // ok
If that is not possible (as with oldof), I could still write:
CONTRACT_CONFIG_DEFINE_OLDOF_KEYWORD // defines oldof Thanks, --Lorenzo
Thanks for the explanation :-)
on Sun Sep 02 2012, Roland Bock <rbock-AT-eudoxos.de> wrote:
Due to a lack of formal contracts in much of day to day programming and the enormous compile time overhead, I don't see the library being used all over the place anytime soon, but I expect it to be used in generic algorithms, helping both developers and library users.
Do we know the sources of the compile-time overhead? If it's mostly in the preprocessor, I suggest consulting with Paul Mensonides. He was an enormous help in getting the preprocessing cost associated with Boost.Python under control. In particular, it's important to make good use of the _D, _R, and _Z suffixes described in http://www.boostpro.com/mplbook/preprocessor.html#id6 -- Dave Abrahams BoostPro Computing Software Development Training http://www.boostpro.com Clang/LLVM/EDG Compilers C++ Boost
On Sun, Sep 16, 2012 at 5:48 PM, Dave Abrahams <dave@boostpro.com> wrote:
on Sun Sep 02 2012, Roland Bock <rbock-AT-eudoxos.de> wrote:
Due to a lack of formal contracts in much of day to day programming and the enormous compile time overhead, I don't see the library being used all over the place anytime soon, but I expect it to be used in generic algorithms, helping both developers and library users.
Do we know the sources of the compile-time overhead?
No, I don't.
If it's mostly in the preprocessor,
I could be in both the preprocessor (either the parsing of the syntax or the expansion of the declaration and contract code) and/or in the template meta-programming code.
I suggest consulting with Paul Mensonides.
I will do some analysis first (probably after the 1st Boost release as the meta-programming optimizations should not alter the public API): http://sourceforge.net/apps/trac/contractpp/ticket/48 Then I will ask the experts as needed--Paul for the pp and yourself for the MPL part ;)
He was an enormous help in getting the preprocessing cost associated with Boost.Python under control. In particular, it's important to make good use of the _D, _R, and _Z suffixes described in http://www.boostpro.com/mplbook/preprocessor.html#id6
IMO, the syntax is "bad" but the compile-time overhead is the real barrier for a broader use of the lib. Therefore, improving compilation time (if at all possible) will be a priority right after the 1st Boost release. Thanks, --Lorenzo
He was an enormous help in getting the preprocessing cost associated with Boost.Python under control. In particular, it's important to make good use of the _D, _R, and _Z suffixes described in http://www.boostpro.com/mplbook/preprocessor.html#id6
IMO, the syntax is "bad" but the compile-time overhead is the real barrier for a broader use of the lib. Therefore, improving compilation time (if at all possible) will be a priority right after the 1st Boost release.
The syntax is this way because of trying to support non-compliant preprocessors. If you look at say the `CHAOS_PP_ENUM_PARAMS` macro(from chaos pp), it doesn't deduce the recursion state, because it is not a higher order macro. Instead, it starts at the upper end of the recursion state and moves downward(whereas higher order macros move upward). That is, `CHAOS_PP_ENUM` moves through the recursion state as 1, 2, 3, 4,... etc, but `CHAOS_PP_ENUM_PARAMS` moves through the recursion state as 512, 511, 510, 509,... etc. This is all possible because of the ability to do recursion through deffered expressions. So, `CHAOS_PP_ENUM_PARAMS` doesn't have to rely on `CHAOS_PP_ENUM` to be implemented because its fairly easy to do recursion. Whereas `BOOST_PP_ENUM_PARAMS` relies on `BOOST_PP_ENUM` and thus there is no way to move throught the recursion state differently. Futhermore, working with a compliant preprocessor can improve performance as well, such as, using pp sequences rather pp lists, and better detection and lookup. However, most of these things can work with MSVC although boost doesn't support it. For example, using this macro here to detect parenthesis(which works in MSVC as well): #define IS_PAREN(x) IS_PAREN_CHECK(IS_PAREN_PROBE x) #define IS_PAREN_CHECK(...) IS_PAREN_CHECK_N(__VA_ARGS__,0) #define IS_PAREN_PROBE(...) ~, 1, #ifndef _MSC_VER #define IS_PAREN_CHECK_N(x, n, ...) n #else // MSVC workarounds #define IS_PAREN_CHECK_RES(x) x #define IS_PAREN_CHECK_II(x, n, ...) n #define IS_PAREN_CHECK_I(x) IS_PAREN_CHECK_RES(IS_PAREN_CHECK_II x) #define IS_PAREN_CHECK_N(...) IS_PAREN_CHECK_I((__VA_ARGS__)) #endif You can now detect if a sequence has zero elements or not, like this: #define SEQ_IS_EMPTY(seq) BOOST_PP_NOT(IS_PAREN(seq)) Also, if you want to check if a keyword is `public`, `private`, or `protected`: #define ACCESSOR_public () #define ACCESSOR_private () #define ACCESSOR_protected () #define IS_ACCESSOR(x) IS_PAREN(BOOST_PP_CAT(ACCESSOR_, x)) Which can be much faster than doing three comparisons with three nested if statements.
Thanks, --Lorenzo
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On Mon, Sep 17, 2012 at 10:46 AM, paul Fultz <pfultz2@yahoo.com> wrote:
He was an enormous help in getting the preprocessing cost associated with Boost.Python under control. In particular, it's important to make good use of the _D, _R, and _Z suffixes described in http://www.boostpro.com/mplbook/preprocessor.html#id6
IMO, the syntax is "bad" but the compile-time overhead is the real barrier for a broader use of the lib. Therefore, improving compilation time (if at all possible) will be a priority right after the 1st Boost release.
The syntax is this way because of trying to support non-compliant preprocessors.
[...] Paul, my apologies for the confusion. I mean "the syntax of Boost.Contract is "bad" but Boost.Contract compile-time overhead is the real barrier...". I was speaking about Boost.Contract and not about Boost.Preprocessor _D, etc. I can see how my reply was confusion in that regard. --Lorenzo
participants (4)
-
Dave Abrahams -
Lorenzo Caminiti -
paul Fultz -
Roland Bock