[boost] [Review:Contract] Late review

Subject: [boost] [Review:Contract] Late review
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2012-09-01 17:59:05

AMDG

First of all, *Yes, Contract should be accepted into Boost*

I've only really looked at part of the documentation
so far.  Here's my comments from that.  I may have
more later depending on how much time I have.

index.html:

- "Contract Programming (CP) allows to specify"
  Use a gerund (specifying) instead of an
  infinitive.  An infinitive here needs a subject.

- "... allowing to find bugs ..."
  Same problem, although it can't be fixed in
  the same way, since the participle + gerund
  would sound awkward.  It could be handled
  with a passive infinitive.  (allowing bugs
  to be found)

- "and it was first introduced by the Eiffel
  programming language "
  You can leave out the "it."  It sounds awkward
  to me.

- "Eiffel programming language"
  Using this repeatedly sounds a bit pedantic.
  You can just say "Eiffel" after the first
  occurance.

- The side by side code in the example gets
  a bit wide.  This will definitely be a
  problem if you want the documentation
  to be generated as a PDF.

- I don't understand why the library needs
  to deal with access control.  Why can't
  the user just use public: private: protected:
  as usual?

- CONTRACT_OLDOF kind of sticks out at me.
  I don't really like the way it looks having
  it a macro when everything else is keywords
  that are parsed by the top level macros.

- "...if there is a bug in the function caller
   for which push_back is called when size is
   equal to max_size ..."
   This clause is somewhat confusing.

- "...with the assertion number to uniquely identity..."
  s/identity/identify/.  Also don't split
  infinitives.

- "This library suffers of two limitations"
  s/of/from/

getting_started.html:

- "This section explains how to setup..."
  "set up" should be two words.  "setup"
  as one word is a noun.

- "... failure of the checked condition does
  not abort the program, instead ..."
  Run on sentence.

- "The implementation of this library uses..., templates with
  partial specializations and function pointers (similarly
  to Boost.Function), ..."
  I don't understand the connection between partial
  specialization and function pointers.  Why are
  they grouped together?

contract_programming_overview.html:

- "It is assumed that programmes"
  s/programmes/programmers/

- "...to formally program specifications".
  Don't split the infinitive.
  "to program specifications formally"

- "Then, this library aims to be ..."
  "Then" doesn't sound right here.  "Then"
  is usually used for conditions ("if... then")
  or sequences (e.g. First.... Then...), but
  there's nothing for it to connect to here.

- "Contract Programming is characterized by the
   following type of assertion mechanisms."
   s/type/types/

- "before the body execution."
  I think it would sound better to use the
  possesive "body's execution" or "execution of the body".
  rather than apposition.

- "Preconditions cannot be strengthen, postconditions and
  class invariants cannot be weaken. "
  s/strengthen/strengthened/, s/weaken/weakened/

- I'm not sure that I would consider Subcontracting
  to be an assertion mechanism.

- "Block Invariants | These are logical conditions that
  programmers except ..."
  s/except/expect/

- "...while a contract assertions is ..."
  s/assertions/assertion/

- "This library implement this feature however
  it should be noted ..."
  Run on sentence.

- "protect such a global variable from racing conditions "
  It's called a "race condition."

- "but that will effectively introduce a global lock
  in the program"
  This seems like a classic use case for thread specific
  storage.

- "Furhtermore,"
  s/Furhtermore/Furthermore/

- "Contracts are part of the source code, they are..."
  Run on sentence (comma splice)

- "detailed error messages that greatly helps debugging"
  "helps" needs to agree with "messages."

- "Contract Programming benefits come to the cost of performance "
  "to" is the wrong preposition.  Try "at."

- "The run-time performances are negatively ..."
  "performance" should be singular.  Plural doesn't
  really make sense for abstract nouns like this.

- "1. The extra processing required to check the assertions.
   2. The extra processing required by the additional
      function calls (additional functions are invoked
      to check preconditions, postconditions, class
      invariants, etc)."
  How is (2) different from (1)?

- "checking function arguments using postconditions "
  I think you mean /pre/conditions.

- "so that these initializations can relay"
  s/relay/rely/

- "C++ object construction mechanism ..."
  You need an article.  "The C++..."

- "Check the non-static class invariants, but only if
  the body threw an exception."
  This could use a rationale.  Destructors are generally
  not supposed to throw at all.

- "Contracts are only responsible to check the "
  "for checking" instead of "to check."

- "...to only use public members, Eiffel instead ..."
  Split infinitive, run-on sentence.

- "...are allowed to brake class invariants..."
  s/brake/break/

- "Arbitrary code in contracts | No, assertions only."
  This is a bit misleading, since assertions can
  execute arbitrary code.

tutorial.html:

- "complete guide on this library syntax"
  s/library/library's/

- result == oldof valule
  s/valule/value/

- "int const"
  IIRC, you can't have a const rvalue of a built-in type.

- "parameter names can instead by omitted "
  s/by/be/

- params_postinc
  You didn't update the pre/post conditions when
  you copied this from the last example.

- "Function and array types cannot be directly used as
  function parameter types within the contract macros
  but extra typedef declarations can be used to
  workaround this limitation "
  This workaround doesn't work in function templates
  where the array element or dimension is deduced.

- "the maximum number of supported array dimensions is
  specified by the CONTRACT_CONFIG_ARRAY_DIMENSION_MAX macro"
  I don't understand the reason for this limitation.

- "...evaluates all contract conditions in
  constant-correct context"
  Add an article "a" before "const-correct."

- "the type of value type"
  Delete the second "type."

- ConstantCopyConstructible
  You're definitition of CopyConstructible is
  wrong.  A type that only defines T(T&) is not
  CopyConstructible.

- "However, it is still necessary to ontract"
  s/ontract/contract/

- unique_identifiers
  I think it's worth pointing out that the postcondition
  can't assume that the precondition was true
  in the face of subcontracting.  I did a double take
  when I saw old_found.

- "Wakened inherited preconditions ..."
  s/Wakened/Weakened/

oldof.hpp:

The definition of CONCEPT_OLDOF is a bit
dangerous.  It can't safely be passed as
a macro argument.

What do you think of this bit of PP magic:

#define TEST_EXPAND_EXPAND_OLDOF(x) CONTRACT_OLDOF
#define TEST_EXPAND_NOTHING(x) oldof
#define TEST_EXPAND_I(x) TEST_EXPAND_ ## x
#define TEST_EXPAND(x) TEST_EXPAND_I(x)
#define oldof TEST_EXPAND(EXPAND_OLDOF(NOTHING(z)))
#define EXPAND_OLDOF(expr) expr

oldof -> expands to itself
EXPAND_OLDOF(random text oldof more random text) ->
 expands to random text CONTRACT_OLDOF more random text

General:

- Are ellipsis parameters allowed?

- Just say "parentheses," not "round parentheses."  Parentheses
  without any qualification always means ().

In Christ,
Steven Watanabe