Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] Update to code rules #99

Draft
wants to merge 2 commits into
base: code
Choose a base branch
from
Draft

[WIP] Update to code rules #99

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e238c15
Updates to code rules from 2020-04-21 meeting
makortel Apr 5, 2021
d91f117
Reword forward declaration rules
makortel Apr 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions cms_coding_rules.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,10 @@ If necessary to create a unique name, one can add the directory name:
`PackageName_SubPackageName_Directory_FileName_h`.
2. Each header file contains one class declaration only. (\*)
3. Header files must not contain any implementation except for class templates and code to be inlined.
4. Do not inline virtual functions.
4. Do not inline virtual functions. (\*) Allowed exception: inlining is permitted for simple functions like accessors and setters, as long as there is at least one non-inline virtual function in the class.
5. Do not inline functions which contain control structures which require block scoping.
6. In your own packages, use forward declarations if they are sufficient.
7. Do not forward-declare an entity from another package.
6. Forward declarations are encouraged in headers within a single package, or wider areas under the same maintenance responsibility.
7. Use headers with forward declarations instead of explicit forward declarions of classes outside of the package or maintenance area.
8. Do not use absolute directory names or relative file paths in `#include` directives.
9. Use `nullptr`, not "0" or "NULL". (\*)
10. Use types like `int`, `uint32_t`, `size_t`, and `ptrdiff_t` consistently and without mixing them.
Expand All @@ -88,10 +88,15 @@ If necessary to create a unique name, one can add the directory name:
27. Provide meaningful argument names in method declarations in the header file to indicate usage, unless the type fully describes the usage.
28. Try to avoid excessively long lines of code that impair readability.
29. Data members of a class must not be redefined in derived classes since doing so hides the original data member and could create confusion between the original and redefined data members.
30. Use `std::abs()` instead of `fabs()`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only abs and not all the math functions ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes why abs only? By the way, clang-tidy has this check https://clang.llvm.org/extra/clang-tidy/checks/performance-type-promotion-in-math-fn.html but this will only apply to math functions with implicit float to double conversion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://blog.audio-tk.com/2018/03/20/writing-custom-checks-for-clang-tidy/ suggests how to add clang-tidy checks for such deprecated functions. @gartung may be we can include these in our llvm build?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes a custom checker can be made for that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gartung , have to looked in to this? Can we add a custom checker for this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry to jump in now in this old thread, but I see developers using fabs instead of std::abs (and even worse using abs for floats) . Was the ckecker discussed above made or is there any objection in moving on with adding this 4.30 rule?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not so sure any more that we should discourage the use of fabs.
In fact, people may end up using a lot more conversions to/from doubles without realising it:

auto x = f(...);            // returns a float
auto y = std::abs(x * 2.);  // returns a double

At least fabs etc. are explicit about being single precision.

Copy link
Contributor

@mmusich mmusich Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not so sure any more that we should discourage the use of fabs.
In fact, people may end up using a lot more conversions to/from doubles without realising it:

good observation, will keep in mind while reviewing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the case about x * 2. should probably be formalized as a rule/reminder that single precision code should rely on single precision literals.
auto y = std::fabs(x * 2.) where x is a float should still return a double; std::fabsf would be a float with a warning if a 2. is used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, yes, fabfs.

31. Use C++ headers, e.g. `#include <cmath>` instead of `#include <math.h>`.
32. Use MessageLogger instead of `std::cout` and `std::cerr` (\*). Allowed exception: standalone programs should use `std::cout` and `std::cerr`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then please fix the message logger poor usability.
Enabling debugging messages for individual modules is at best a pain.

33. Avoid `thread_local`.

## 5 -- Documentation Rules
1. Always comment complex, tricky, or non-intuitive portions of code.
2. When revising code, be sure to update and revise comments.
3. Do not leave commented-out code in files. Instead, use git versioning to retain old versions of code in the development history. If code is only commented out temporarily, a clear comment should be added describing why it is temporarily disabled and under what conditions it will be re-enabled.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "use git versioning to retain old versions of code" assumes that one knows to look in the git history for older versions of the code - people other than the original developer (who may very well have left CMS by the time the code is looked at again) may have no idea that an alternative version was ever available.
In addition, the very loose approach to a git history used in CMSSW makes it often very hard to figure out which commits contain useful code, and which ones are just iteration of non-working code before the final version is merged.

In addition, some code make a lot more sense being commented out than outright removed. For example, commented-out debug statements tell the people that read he code how to check some internal states, etc.
To those that advocate using #ifdefs for this, please explain how

#ifdef SOME_RANDOM_DEBUG_FLAG
    std::cout << "internal state: " << myObject.state() << std::endl;
#endif  // SOME_RANDOM_DEBUG_FLAG

is more readable or better maintainable than

    //std::cout << "internal state: " << myObject.state() << std::endl;

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more maintainable because the debug statement can be activated without editing the code. (I agree that the readability is not great.)

The key point is really in the last sentence of this code rule: files littered with commented-out code are hard to understand for someone looking at them for the first time, because it's usually not indicated why the commented-out code is there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree for the case where there are a bunch of statements that could be something somebody has tried and didn't work, or something that was there before and has been changed, etc.

However I don't think that

    //std::cout << "internal state: " << myObject.state() << std::endl;

is hard to understand - neither why the comment is there, nor what it would do if uncommented.

It's more maintainable because the debug statement can be activated without editing the code.

True - unless there are #define/#undef for that symbol in the source file, then it does need to be edited.
Or unless you want to enable only one of those messages, not all of them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion

//std::cout << "internal state: " << myObject.state() << std::endl;

is really hard to understand :-) This does not tell me if it was added during the development phase and user forgot to remove it or if it was really to indicate the it is for debugging purpose in future. Better to either use #ifdef or add a comment to indicate that why this comment is there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we disagree.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, for this specific example an ideal solution would be to have a usable system for debug statements, with a clear syntax, support for the most common idioms (not only the ones that the Core Software team deems "appropriate" for the developers to use), and a simply way of turning the debug information on and off.


## 6 -- Packaging Rules
#### Libraries
Expand Down Expand Up @@ -148,3 +153,5 @@ These guidelines are a brief summary of highlights from the [C++ Core Guidelines
14. Design functions that are short and simple and that perform a single, coherent, logical task ([logical task](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-logical), [short functions](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-single)).
15. Do not duplicate code. If procedural code is needed in two or more places, create a function for the task and call it where needed. ([functions](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-package), [encapsulate](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rp-library))
16. Do not use goto ([no goto](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-goto)).
17. Do not use `using namespace` in global scope in a header file ([no using namespace in headers](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rs-using-directive).
18. Avoid `const_cast` ([const cast](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-casts-const)).