P1666 review issues from OFFIS

Philipp,

Here is my response to your review issues, excluding those issues for
which I consider further discussion is definitely required. Of course, you
are free to dispute the issues below where I consider no change is
required.

My comments are marked [JA] below

John A

4.3.4 Namespaces and internal naming

  The clarification adds 'and definition' to the required namespace
  location of added names. Since every (regular) definition is a
  declaration as well, this seems redundant.
 
[JA] Agreed. Changed.

  Moreover, a preprocessor macro _definition_ (#define) may suddenly
  be covered by this as well, which can not be placed in any namespace.
  Should there be a note about this?
 
[JA] Agreed. "Declaration" is sufficient.

  Note: Alan already mentioned the probably missing 'sc_unnamed'
  namespace. sc_unnamed::_X should not be imported to the global
  namespace by "systemc.h".

6.1.1 #include "systemc"

  "The header file named systemc shall add the names sc_core and sc_dt
   to the declarative region in which it is included, _and_these_two_
   _names_only_."

   -> sc_unnamed, sc_main declaration (optional) missing

[JA] I have added sc_unnamed but not sc_main, because sc_main is provided
by the application, not the implementation.

  "... shall not introduce ... any other names from this standard or
   any names from the standard C or C++ libraries"

  Obviously, this can't be fulfilled by any implementation, if
  'systemc' is the only included file. At least for e.g. 'sc_bind',
  get_child_objects(), or 'sc_exception', other names are definitely
  required (at least 'boost' and 'std').
 
[JA] The intent was to say that the implementation should not introduce
unqualified names from the std or boost namespaces, although those names
are of course still accessible using explicit qualification (namely
std::vector, std::string, std::istream, std::ostream, boost::bind,
boost::ref, boost::cref). So I am not yet convinced the LRM is wrong,
although the wording could always be improved. What do you think?

  I also don't know how to handle macros here.
  They are a different kind of 'names', right?

[JA] Agreed, but is there any need to clarify this?

6.9.2 sc_event_and_expr, sc_event_or_expr definition

  Two free-standing operators missing:

  sc_event_and_expr operator& ( sc_event_and_expr
                              , sc_event_and_list const& );

  sc_event_or_expr operator| ( sc_event_or_expr
                             , sc_event_or_list const& );

[JA] Agreed. My mistake. Changed.

6.9.3 Constraints on usage (sc_event_*_expr)

 "The implementation shall delete the event expression object when the
  process instance resumes or is triggered as a direct result of the
  notification of one or more events in the event expression. In
  other words, the implementation shall be responsible for the
  creation, deletion, and memory management of event expression objects
  of type sc_event_and_expr? and sc_event_or_expr?."

 This seems to be a bit imprecise. The event expression object is
 actually a temporary, returned by the final operator in the event
 expression (no pun intended). It is therefore destroyed at the end of
 the full expression (e.g. the call to wait()).

 The thing that is actually managed by the application is the
 creation, deletion and memory management of the returned 'event
 list', via 'operator sc_event_and/or_list const&'.

[JA] Agreed. I got event lists confused with event expressions. Changed.

6.12.5 (sc_port constructors)

  ~sc_port_b(); -> "The destructor shall do nothing."

    Is this restriction needed?
 
[JA] I agree it would be more consistent (with the rest of the LRM) to
omit this. Changed.

  sc_port();
  sc_port( const char* );

    The sc_port constructors should explicitly pass their template
    parameters N, and P to the constructor of the new sc_port_b base
    class.
 
[JA] Agreed. Well spotted. Changed.

7.30.2/7.30.4 sc_event_queue constructor

  The explicit cast to sc_module_name around the sc_gen_unique_name()
  call is not needed.
 
[JA] I agree the LRM is over-specified on this point (it was copied
straight from the source code), but do we care enough to want to refine
the description? (In writing the original LRM we paid less attention to
the more peripheral classes.)

9.5.2 sc_vector (class definition)

  sc_vector<T>::size_type is redundant, sc_vector_base::size_type
  should suffice.
 
[JA] Okay. Changed.

  It might be a good idea to make 'sc_vector_iter<T>' a daggered
  name, since one can (and should) always use sc_vector<T>::iterator
  instead.
 
[JA] Okay. Will do.

 For consistency with the other macro definitions, a hint that a
 trailing semicolon after the macro is required would be good.

[JA] Agreed. Done.

Annex B, B.58

  "event expression: A list of events, separated by either operator& or
   operator|, and passed as an argument to either the wait or the
   next_trigger method."

  Should be: A list of events or event lists, separated ...
 
[JA] Agreed. Changed.

 "Applications are recommended to call function sc_stop before
  returning control from sc_main to ensure that the end_of_simulation
  callbacks are called."

 Probably add a forward reference to 5.4.4.

[JA] Agreed. Changed.

5.4.1 before_end_of_elaboration

 "The following constructs shall not be used directly within member
  function before_end_of_elaboration,?
  a) the macro SC_CTOR"

 The previous paragraph has been rephrased to restrict the use of
 other functions within before_end_of_elaboration.

  SC_CTOR _declares_ a constructor (and adds some impl-defined
 definitions), it can't be used within a function anyhow. So maybe we
 should drop this entire second paragraph here?
 
[JA] The point is in the part you elided, namely "but may be used where
permitted within module instances nested within callbacks to
before_end_of_elaboration". Hence I think the point still stands. However,
I will change "shall not be used" to "cannot be used" (since a
constructor cannot be declared within a function).

6.6.8 sc_is_unwinding

  In the example, 'class wait_on_exit' should probably be a 'struct',
 since the destructor is otherwise private. Missed it in my original
 example.

[JA] Agreed. Changed.

6.10.2 sc_event - Class definition

  Constructor 'sc_event( const char* )' should be 'explicit'.
 
[JA] Agreed. Changed.

6.16 sc_object

   Shouldn't there be a get_child_events() in sc_object already,
  returning an empty event list by default? Otherwise, traversals
  require explicit casts to sc_module and/or the creation of a process
  handle to obtain child events.
 
[JA] Yes, good point. Done.

8.10.12.2/8.10.13.2 sc_fxval(_fast) - Class definition

   Sorry, my fault. I should have double-checked the LRM last time.
  The constructors of sc_fxval(_fast) for the "other" integral types
  are already explicit in the reference simulator but not in the LRM:

    int64, uint64, sc_(u)int_base, sc_(un)signed

  They need to be explicit as well.
 
[JA] Okay. Done.

9.3.5 sc_report_handler :: report

  "The macros SC_REPORT_INFO, SC_REPORT_WARNING, SC_REPORT_ERROR,
   SC_REPORT_FATAL,?"

  -> SC_REPORT_INFO_VERB missing

[JA] Agreed. Changed.

-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.