[WIP] Update to code rules

[makortel]

Changes in contents that have been brought up in #94 (up to further discussion).

[fwyzard]

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;

?

[kpedro88]

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.

[fwyzard]

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.

[smuzaffar]

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.

[fwyzard]

Then we disagree.

[fwyzard]

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.

[fwyzard]

Why only abs and not all the math functions ?

[smuzaffar]

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.

[smuzaffar]

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?

[gartung]

Yes a custom checker can be made for that.

[smuzaffar]

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

[mmusich]

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?

[fwyzard]

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.

[mmusich]

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.

[slava77]

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.

[fwyzard]

Sorry, yes, fabfs.

[fwyzard]

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

[makortel]

We should probably revive doing these updates, but in smaller chunks (maybe even one at a time) to be able to conclude.