Home » Blogs » jeromel's blog

Comments on the STAR C++ coding guidelines

Updated on Tue, 2015-03-17 13:24. Originally created by jeromel on 2015-03-17 07:09.
See also You do not have access to view this node

Comments from 2015/03/17
  • I think the document is well written and very detailed and a huge step forward for STAR as far as coding standards and guideline go. I would like to congratulate the committee for their work. I very much like the embedded references to standard and common problems (rule of three, rule of five, diamond problem, etc.). I feel the guidance will go a long way to help casual developer get a better grasp on the language semantics and features as well as the reasoning behind the guidelines.
     
  • A few comments follows - they will be flagged sometimes as cosmetic
     
  • [cosmetic] whenever an arrow is expanded, a wod "link" appears on the left hand side as showed below

    However, clicking on this link would drive us at the top of the document, making the purpose for this tag unclear.
     
  • [clarification] Namespace versus Scoping sections may confuse readers. In the first, namespacing is discouraged. The scoping section starts stating "Nonmember functions... should be within a namespace.". Perhaps the note of an exception to namespacing?
     
  • [confusion] 
    auto value = double{1.23};
    is noted as good. I would consider this example as to-avoid-at-all-cost (and prefer double value = 1.23;). This may inadvertently encourage the use of auto. I would suggest to skip this special auto case (or did I miss something?).
     
  • [cosmetic] In the provided example below

    the first class code bock could be indented as the second for comparison readability.
     
  • [cosmetic] In section "Classes" -> "Exception 2", " constructor and assignment operators should be be disabled" has an extraneous "be".
     
  • [cosmetic] In the inheritance section, the side example (C++11 box) 
     void a(float) override; // doesn't override Base::a(int) (wrong signature)
    should probably be color coded red as all previous wrong/bad use examples. There are more examples in that block and throughout the document (i.e. where color coding of bad style would probably enhance readability).
     
  • [clarification] I am not sure of the logic behind a virtual final functor in a class declaration ... when would this be needed if at all? Is this the example intended?
     
  • [precision] In the ROOT type section, the allowable cases of use of ROOT types is unclear. Data members that would be IO streamed should use ROOT types to avoid IO (and schema evolution) issues. As stated, it is not clear (there is a reference to "persistent" classes but it is not all clear that readers will understand where/when to use ROOT type).
    Perhaps a specific note about IO and streamers should be made in this context?
     
  • [comment] I strongly agree with the comment related to C++ math function versus ROOT ones. In addition, I would recommend users to think twice before using a class for simple operations. I remember an example in PHENIX where a 3D vector class was used to ... calculate a distance. Each call would create/destroy a class for essentially doing a sqrt(x^2+y^2+z^2) ... Most of the code (chain) time was spent in that alone (replacing by a simple arithmetics caused a net 15% overall speed gain). Abuse of classes where not necessary may have pernicious side effects ...
     
  • [precision] I am worried of C++ exception (as also noted in the document). In the exception handling section, I would welcome the following recommendations (if disagree, please explain):
    • Don't hide exception by using a catch followed by an empty code block
    • Don't EVER use exception to indicate the absence of a resource
    • Similarly, don't use exception as a mean to return special information from a method
    • Use exception ONLY for errors that should not be ignored
    • Don't clear the stack trace by re-throwing an exception i.e. 
      catch (Exception ex)
{
      // some code here
      throw ex;
}
      should not be used as "throw ex" will throw a new exception and clear the stack trace (bad for code developers). Instead, use
      catch (Exception ex)
{
      // some code here
      throw;
}
      which will not clear the stack.
    • ...
       
  • [cosmetics] In "Inline namespaces and Align Statement", a left side arrow appears but clicking on it would lead to no additional information (there are other places where this is is true / only need to check if intended).
     
  • [problem] In the casting section, I am not for the wording "C-casts are forbidden" and would prefer "C-casts should be avoided and are highly discouraged" (or similar). An alternative could be to the C-cast is forbidden, expand with something like "If there is an absolute need for a C-casting that cannot be fulfilled by a static_cast or a dynamic_cast, consider the use of reinterpret_cast". It is explained later but only if one opens the details (and read quite a bit through it).

    As already noted in my previous wave of comments, either C-casts or reinterpret_cast will be needed to allow the online world of "reading bank and moving pointer" to co-exists with our offline framework (which we need to remember is a "single framework does it all"). The use of char * casting to (MyGrandMa *)  :-)  is numerous and often a necessity as the pointer in a data-bank will point to many different object as the data structure is inspected (there are often marker variables to indciate beging/end of each structure).
     
  • [cosmetics] In the section "Increment and decrement operator", an arrow appears but no more details appear if one clicks on it (revisit as noted above).
     
  • [problem] In the example provided for loop, an assert() statement is showed for the default: case. assert() have been strongly discouraged in STAR and I would recommend using another example here. It is not consistent with our previous set of recommendations I append below verbatim:

    • To exit your code on an error condition in the STAR framework, return one of the STAR return codes (an enum) from your Maker: 
      enum EReturnCodes{
  kStOK=0,  // OK
  kStOk=0,  // OK
  kStWarn,  // Warning, something wrong but work can be continued
  kStEOF,  // End Of File
  kStErr,  // Error, drop this and go to the next event
  kStFatal      // Fatal error, processing impossible
};

      Outside Makers in your application code AND for testing conditions that should never happen and indicate disaster if they do, we recommend the use of assert() (it aborts the program if the assertion is false). Don't use exit(). For info on assert() see the man page ('man assert').
      Generally speaking though, it is BAD to just abort the program and should be avoided. Better to message an error and propagate up a fatal error flag. Unless your Maker is a fundamental and base maker (DAQ detecting a corruption, db finding an unlikely condition which should really never happen, IO maker not able to stream, ...) the use of assert() is prohibited as a single detector sub-system error shall not lead to an entire chain abort. The use of clear messages with level FATAL is emphasized.

  • [missing] There are many sections from our "C++ Programming Guidelines" that are not currently in the new document. Examples includes
    • The use of the STAR logger
    • Comparison of pointer to int (a classic including  in the form if ( p == 0){} )
    • The current document speaks of avoiding ROOT types for the benefit of C++ types but we recommended the use of StTypes before (shall we deprecate?)
    • Recommendation regarding passing an object passed by reference (but not modified) as const
    • Use of return code (kStOK, kStWarn, etc) from Makers
    • The use of assert() [already noted above as a consistency issue]
    • Self-explanatory English variable names
    • Camel style naming (many bullets in our initial guidelines)
    • ...

»