smirnovd's blog

[general comment] Thank you for providing these extensive drafts with the new coding guidelines for STAR
- In some sections the draft looks a little too overloaded with details. Perhaps, explaining the language features should not be the purpose of this document. For example, I am not even sure that such rare features as "specifier attributes" should be mentioned. Please note that in the context of our existing software, some of our contributors don't think twice what to write in the log file, such beyond-fine-tuning features are not very relevant.
- It may be a good idea to thin out or completely remove sections on "lvalue/rvalue", "specifier attributes", "variable length arrays", "=default and =delete" keywords. These are for fine tuning and optimization, and are not intended for an average physics professor or a grad student.
- Don't get me wrong: If someone wants to use any of the above, I would love to hear his/her arguments about it. In general, I am against forbidding any of the language features :)

[style] With current style all headers (sections, subsections, ...) look alike. Please make it easier for the reader to visually separate individual sections. You can at least increase the font of the main headers.

[recommendation] I would recommend to add a suggestion for running an automatic style formatting tool such as astyle before the code is committed. In my opinion, the specific style is not important and can be discussed but I think it should be close to what we already have in STAR. My suggestion is to use:
```
$astyle -s3 -p -H -A3 -k3 -O -o -y -Y -f [.h/.cxx file(s)]
```

[missing] I don't see anything related to lambda expressions. Was it removed on purpose? The short section in the original google guide looks very appropriate to me.
- [addressed]

[recommendation] I would question the recommendation to have an empty destructor in every class. In my opinion, a destructor (in fact, any class method) should be provided only if does something useful. If there are no resources to delete then there is no need to add extra code. The argument that this requirement helps to not forget to clean up allocated resources seems rather weak and does not seem to be working from what I have seen in the code reviews. On contrary, an empty destructor may look like the author intended to delete something but forgot.
- [addressed] relevant change

[recommendation] Another comment is about your strong suggestion to not define methods in the class body. For one-liners such as simple setters/getters it is quite natural from the point of view of the code writer and the reader to have them defined in the class body. I see very little reason to discourage such practice.

[recommendation] As a general guideline, I would add a recommendation to not have commented out code in the production code. A commented code is a rudiment from the times when there were no sophisticated revision control systems which we have now. The developer would stash a feature for later consideration right there in the code but often would forget about it. The other developers do not care about such unfinished or under-implemented feature which just draws their attention from the real code. Thankfully, the modern revision control systems allow us to save and test new features on branches without polluting the main trunk with commented code.

[suggestion] I suggest to get rid of the archaic CVS keywords in the code. The most harmful is $Log$ at the begining of files because it makes patching more difficult when expanded. Also, when working with branches the history of file changes is not linear and printing it as a list is confusing.

[suggestion] assert()'s should be discourage in the production code. assert()'s are for debugging only. For example, killing a job with assert() when encounter a rare event is not smart. The developer should issue an error/warning message and investigate.

[suggestion] Smart pointers should not be discouraged in STAR software. Possible conflicts with ROOT can be considered on case by case basis.

The STAR experiment