Updates for Code rules #94
Comments
|
I was thinking to address these myself, but wanted to open an issue in the mean time. |
|
while we're here, another informal rule should be formalized: use maybe also: "use c++ headers like rather than c headers like <math.h>"? |
|
Do the Core Guidelines say anything about either? |
|
avoiding i don't see anything about c++ vs c headers, but i think this is also something we commonly recommend |
|
I know and agree, I was just wondering if the Core Guidelines would have a recommendation we could refer to (sorry I was unclear). |
|
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 |
|
i also notice that our ban on |
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). |
|
another one that I don't see explicitly in the rules:
|
|
could also add "avoid const_cast" |
|
Proposal: relax
to something like
|
|
Is it written somewhere about avoiding |
|
It is kind of part of original CMs coding huide lines document |
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
) |
|
Maybe we should add something about |
|
what about (currently 4.7) |
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? |
|
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 GitHub<#94 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGPFQ3V4B5BHTFGMET5FUTSBZ46XANCNFSM4MEX3ZIQ>.
|
|
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 |
Yes, including headers of forward declarations is fine. |
Let's say there is a type 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 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). |
|
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 @wddgit<https://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 GitHub<#94 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGPFQ3RG3CCS4LEEGFJG4DSB2F7HANCNFSM4MEX3ZIQ>.
|
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? |
|
Using forward declarations is a very easy way to cause 1 definition rule violations (the forward declaration declares a 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. |
|
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 GitHub<#94 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGPFQ744U3T6MQVA6L3HDTSB2LJFANCNFSM4MEX3ZIQ>.
|
|
Which of these changes (and of the existing rules) can be implemented in 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 |
|
|
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") |
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)
--too much)std::abs()instead offabs()"#include <cmath>instead of#include math.h"coutorcerr, use e.g. MessageLogger instead"const_cast"thread_local"The text was updated successfully, but these errors were encountered: