RungeKuttaUtils : A reference can not be null

RungeKuttaUtils: A reference can not be null

C++ std

A reference is required to be initialized to refer to a valid object or function: see reference initialization. Note: in particular, a null reference cannot exist in a well-defined program, because the only way to create such a reference would be to bind it to the “object” obtained by dereferencing a null pointer, which causes undefined behavior.

This merge request affects 1 package:

• Tracking/TrkExtrapolation/TrkExUtils

This merge request affects 1 file:

• Tracking/TrkExtrapolation/TrkExUtils/src/RungeKuttaUtils.cxx

Adding @gavrilen as watcher

added Tracking main review-pending-level-1 labels

The system selected 56 tests to probe the Athena changeset (out of 65 available tests). Link to tests selection rules

CI Result SUCCESS (hash c39edde3)

	Athena
externals
cmake
make
tests

Full details available on this CI monitor view. Check the JIRA CI status board for known problems
Athena: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 79404]

Escalating to L2, since I do not fully understand the newly returned functions in the new lines.

Cheers, Philipp (L1)

removed review-pending-level-1 label

added review-pending-level-2 label

It seems the check on the validity of the passed Trk::TrackParameters is not performed any more. The reason of this could be explained by the title of this MR.

Assuming that this is exactly what it is intended to be done, changes look fine, and approval could be granted.

Nevertheless, it could be worth making the description more explanatory, so that the changes introduced are better understandable. Changing then to the user action label.

added review-user-action-required label and removed review-pending-level-2 label

Hi @grancagn . The check if useless as a

"reference can not be a nullptr...." .

If it was the should not vs can not that was not strong enough I changed it.

In short

the check on the validity of the passed Trk::TrackParameters is not performed any more.

1. There was never a check on the validaty of the passed (by const ref) parameters.

2. There was a check on if a ptr to a valid initialized object can be null.... Which is another thing all together. This is removed in this MR.

The reason of this could be explained by the title of this MR.

The answer is that this can/should not be null.

This comes from the std of the language :

A reference is required to be initialized to refer to a valid object or function: see reference initialization.

Note: in particular, a null reference cannot exist in a well-defined program, because the only way to create such a reference would be to bind it to the “object” obtained by dereferencing a null pointer, which causes undefined behavior.

What happens (seen in the changeset) here is :

const Trk::TrackParameters& Tp //passed by reference 

and then then there was a check (removed) that was somehwere between meaningless, confusing undefined behaviour (expand here ... )

  const Trk::TrackParameters* pTp = &Tp;  // take address of object. 
  if (!pTp) // check if ptr to an object is non nullptr /valid ?
    return false; // can never happen 

In any case all the description says is what it happens.

The "reference" should/can not be a null.

Assuming that this is exactly what it is intended to be done, changes look fine, and approval could be granted.

Absolutely and I assume if it can confuse L1 and even L2 shifters being done is also quite evil ....

If people actually assumed that what was removed was doing something , like checking validity of input, and is not UB, this could lead to these instead of cleaned - up , being entrenched and multiply ...

PS you can play with things like

https://godbolt.org/z/Yfo5hjEos the cout is never emitted the full if logic is omitted. Even in debug not opt it will be omitted https://godbolt.org/z/zdbEn1j6x.

As it evokes undefiend behaviour the compiler can do in principle few things. Usually it will not emit the check

changed title from RungeKuttaUtils reference should not be nullptr to RungeKuttaUtils reference can not be nullptr

changed the description

removed review-user-action-required label

added review-pending-level-1 label

changed title from RungeKuttaUtils reference can not be nullptr to RungeKuttaUtils : A reference can not be nullptr

changed title from RungeKuttaUtils : A reference can not be nullptr to RungeKuttaUtils : A reference can not be null

changed the description

added review-approved label and removed review-pending-level-1 label

resolved all threads

merged

mentioned in commit fe845e4e

added sweep:ignore label