Updates for Code rules

[makortel]

The Code rules (http://cms-sw.github.io/cms_coding_rules.html) would benefit from some updates (list has been updated based on the discussion below)

• The links in the Table of Contents are broken (they have -- too much)
• Links to C++ Core Guidelines could updated to point to http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines instead of https://github.com/isocpp/CppCoreGuidelines
• Add SF.7: Don’t write using namespace at global scope in a header file from the C++ Core Guidelines under Section 7. (spurred by https://github.com/cms-sw/cmssw/pull/29381#discussion_r406217323)
• Add "Use std::abs() instead of fabs()"
• Add "Use C++ headers, e.g. #include <cmath> instead of #include math.h"
• Add "Do not use cout or cerr, use e.g. MessageLogger instead"
• Add "Use git versioning to retain old versions of code in the development history, rather than leaving commented-out code in files. if code is only commented out temporarily, a clear comment should be added describing why it's temporarily disabled and under what conditions it will be re-enabled."
• Add "Avoid const_cast"
• Relax 4.4. "Do not inline virtual functions." to "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."
• Add "Avoid thread_local"

[makortel]

I was thinking to address these myself, but wanted to open an issue in the mean time.

[kpedro88]

while we're here, another informal rule should be formalized: use std::abs() not fabs()

maybe also: "use c++ headers like rather than c headers like <math.h>"?

[makortel]

Do the Core Guidelines say anything about either?

[kpedro88]

avoiding fabs is something we've recommended for a while to prevent undesirable type conversions (since c++11 improved std::abs())

i don't see anything about c++ vs c headers, but i think this is also something we commonly recommend

[makortel]

I know and agree, I was just wondering if the Core Guidelines would have a recommendation we could refer to (sorry I was unclear).

[smuzaffar]

For c vs c++ headers, although there is nothing in the Core guidelines but we force it via clang-tidy check https://clang.llvm.org/extra/clang-tidy/checks/modernize-deprecated-headers.html

[kpedro88]

i also notice that our ban on cout is not described in the code rules, this should definitely be added

[makortel]

i also notice that our ban on cout is not described in the code rules, this should definitely be added

It sort-of falls under 7.2 ("Ensure code is thread-safe"), but yes, it would be clearer to mention it explicitly.

Sounds like there is room for gather a bit more stuff and do one larger update (with a presentation in the core meeting and proper announcement of the update).

[kpedro88]

another one that I don't see explicitly in the rules:

• use git versioning to retain old versions of code in the development history, rather than leaving commented-out code in files. if code is only commented out temporarily, a clear comment should be added describing why it's temporarily disabled and under what conditions it will be re-enabled.

[kpedro88]

could also add "avoid const_cast"

[fwyzard]

Proposal: relax

Do not to inline virtual functions

to something like

Do not to inline virtual functions (*)

(*) 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

[silviodonato]

Is it written somewhere about avoiding using namespace in header files?

[smuzaffar]

It is kind of part of original CMs coding huide lines document https://cds.cern.ch/record/687032/files/note98_070.pdf

Use ‘using namespace’ only in local scope.

[makortel]

Is it written somewhere about avoiding using namespace in header files?

It is not part of our current code rules (although our tools flag it), which is why I'm suggesting to add it (from issue description

• Add SF.7: Don’t write using namespace at global scope in a header file from the C++ Core Guidelines under Section 7. (spurred by cms-sw/cmssw#29381 (comment))

)

[makortel]

Maybe we should add something about thread_local as well.

[slava77]

what about (currently 4.7) Do not forward-declare an entity from another package. ? It seems to be systematically violated. Should some note like not really enforced and is widely not followed be added?

[makortel]

what about (currently 4.7) Do not forward-declare an entity from another package. ? It seems to be systematically violated. Should some note like not really enforced and is widely not followed be added?

Would that be much different from removing the rule? (ok, by not removing someone could still adhere the rule, which would be beneficial)

I think we should also consider actually enforcing the rule, at least on new code. Not following it has a risk of increasing maintenance cost.

[slava77]

what about (currently 4.7) Do not forward-declare an entity from another package. ? It seems to be systematically violated. Should some note like not really enforced and is widely not followed be added?

Would that be much different from removing the rule? (ok, by not removing someone could still adhere the rule, which would be beneficial)

I think we should also consider actually enforcing the rule, at least on new code. Not following it has a risk of increasing maintenance cost.

BTW, where is the maintenance cost, and why is rule 4.6 allowing forward declarations from the same package? The obvious explicit maintenance cost is that users of the package have to include header files for the forward-declared types when such type members are needed. But according to rule 4.6 I'd guess that this cost is not really counted. So, what remains is the cost of maintaining the BuildFile dependency declarations. Would it be OK then to allow the forward declarations from other packages if the package itself exports dependency on that package?

[davidlange6]

I’m also curious as to the motivation of 4.7? If you include what you use, and add dependencies in BuildFiles of your #includes, what possible error comes up?

On Aug 21, 2020, at 4:02 PM, Slava Krutelyov <[email protected]mailto:[email protected]> wrote:

what about (currently 4.7) Do not forward-declare an entity from another package. ? It seems to be systematically violated. Should some note like not really enforced and is widely not followed be added?

Would that be much different from removing the rule? (ok, by not removing someone could still adhere the rule, which would be beneficial)

I think we should also consider actually enforcing the rule, at least on new code. Not following it has a risk of increasing maintenance cost.

BTW, where is the maintenance cost, and why is rule 4.6 allowing forward declarations from the same package? The obvious explicit maintenance cost is that users of the package have to include header files for the forward-declared types when such type members are needed. But according to rule 4.6 I'd guess that this cost is not really counted. So, what remains is the cost of maintaining the BuildFile dependency declarations. Would it be OK then to allow the forward declarations from other packages if the package itself exports dependency on that package?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/cms-sw/cms-sw.github.io/issues/94#issuecomment-678306422, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABGPFQ3V4B5BHTFGMET5FUTSBZ46XANCNFSM4MEX3ZIQ.

[wddgit]

If we are actually considering enforcing this rule, we might want to consider making some exceptions.

I think we are allowed to forward declare by including a header file like FWCore/Framework/interface/Frameworkfwd.h, even outside FWCore/Framework. Is this true?

There has been some discussion in the past that a related group of packages maintained by the same people can forward declare classes from other packages in the same group. The Core packages have many such forward declarations. ( I am guilty of adding many of them, although not all and I think I started doing that by copying the pattern already there long ago before this rule existed). Such a definition would be harder to enforce because we would have to define the groups of packages...

Are there other reasonable exceptions?

When I was originally learned C++ I was strongly encouraged to use forward declarations and you still see that advice some places.

Some of the pros and cons are discussed here: https://google.github.io/styleguide/cppguide.html#Forward_Declarations

[makortel]

I think we are allowed to forward declare by including a header file like FWCore/Framework/interface/Frameworkfwd.h, even outside FWCore/Framework. Is this true?

Yes, including headers of forward declarations is fine.

[makortel]

BTW, where is the maintenance cost, and why is rule 4.6 allowing forward declarations from the same package?

Let's say there is a type Foo that is a class. For some reason the implementation needs to be changed to a class template, such that a type alias along using Foo = FooTemplate<SomeType>. Now all the forward declarations along

class Foo;

need to be changed to

template <typename T>
class FooTemplate;
using Foo = FooTemplate<SomeType>;

If the forward declarations are scattered around in many places outside of the package defining the type, that would require a lot of work.

If the forward declarations are placed in one header for the package for the clients of that package to include, it is sufficient for the package maintainer to cover all clients by just updating that header (like FWCore/Framework/interface/Frameworkfwd.h that @wddgit mentioned).

Forward declarations within a package can be thought of being up to the package maintainer(s) to decide how to deal with (scattering the declarations vs. gathering them into a header).

[davidlange6]

So yes I agree this is a case where the forward declarations would have to be changed for an otherwise “invisible” change. But it seems to me that this sort of benefit is small compared to the cost (in a system with >1000 packages), as it is what appears to me as a corner case.

On Aug 21, 2020, at 5:19 PM, Matti Kortelainen <[email protected]mailto:[email protected]> wrote:

BTW, where is the maintenance cost, and why is rule 4.6 allowing forward declarations from the same package?

Let's say there is a type Foo that is a class. For some reason the implementation needs to be changed to a class template, such that a type alias along using Foo = FooTemplate<SomeType>. Now all the forward declarations along

class Foo;

need to be changed to

template <typename T> class FooTemplate; using Foo = FooTemplate<SomeType>;

If the forward declarations are scattered around in many places outside of the package defining the type, that would require a lot of work.

If the forward declarations are placed in one header for the package for the clients of that package to include, it is sufficient for the package maintainer to cover all clients by just updating that header (like FWCore/Framework/interface/Frameworkfwd.h that @wddgithttps://github.com/wddgit mentioned).

Forward declarations within a package can be thought of being up to the package maintainer(s) to decide how to deal with (scattering the declarations vs. gathering them into a header).

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/cms-sw/cms-sw.github.io/issues/94#issuecomment-678346716, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABGPFQ3RG3CCS4LEEGFJG4DSB2F7HANCNFSM4MEX3ZIQ.

[makortel]

But it seems to me that this sort of benefit is small compared to the cost (in a system with >1000 packages), as it is what appears to me as a corner case.

Could you elaborate what you refer to with "cost"? Cost of changing the inter-package forward declarations to go via headers? Cost of forward declarations in general? Something else?

[Dr15Jones]

Using forward declarations is a very easy way to cause 1 definition rule violations (the forward declaration declares a class but the name was changed to be a typedef). I've actually encountered this quite a number of times when changing (or fixing) CMSSW code.

Given that forward declarations are useful for decoupling systems (which we do want to do) then the safest way the C++ programming community has found to use them is to have forward declaring headers where that header lives in the same software area (e.g. for CMSSW a package) as the full header. Our coding rules are meant to reflect what the C++ community sees as best practices.

[davidlange6]

Perhaps we can recast the rule this way and think to enforce it? Eg, forwarding headers shall be provided in lieu of any forward declarations outside of the package?

On Aug 21, 2020, at 6:04 PM, Chris Jones <[email protected]mailto:[email protected]> wrote:

Using forward declarations is a very easy way to cause 1 definition rule violations (the forward declaration declares a class but the name was changed to be a typedef). I've actually encountered this quite a number of times when changing (or fixing) CMSSW code.

Given that forward declarations are useful for decoupling systems (which we do want to do) then the safest way the C++ programming community has found to use them is to have forward declaring headers where that header lives in the same software area (e.g. for CMSSW a package) as the full header. Our coding rules are meant to reflect what the C++ community sees as best practices.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/cms-sw/cms-sw.github.io/issues/94#issuecomment-678368336, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABGPFQ744U3T6MQVA6L3HDTSB2LJFANCNFSM4MEX3ZIQ.

[fwyzard]

Which of these changes (and of the existing rules) can be implemented in clang-tidy, even without a "fix up" rule ?

I assume most L2s have better ways to spend their time than making the same suggestions over and over again.

Also, can we make at least the error reporting (without the automatic fixes) part of scram b ?

[smuzaffar]

• Add "Use C++ headers, e.g. #include <cmath> instead of #include math.h
  • this is already taken care of by https://clang.llvm.org/extra/clang-tidy/checks/modernize-deprecated-headers.html
• Add "Use std::abs() instead of fabs()"
  • Can be partially implemented by https://clang.llvm.org/extra/clang-tidy/checks/performance-type-promotion-in-math-fn.html ( but only if float argument are passed)

[makortel]

To make some progress I made two PRs of the updates discussed here: #98 for updating the links in TOC and to C++ core guidelines, and #99 for the actual contents update.

Maybe we should go through the rules systematically and try to add automated checks. Maybe even denote the level of enforcement in the rules? (e.g. along "by eye in review", "checked without fixup", "checked with fixup")