Making IGeoSvc::getDetector and constantAsString const.

[scott-snyder]

We want to make the methods of IGeoSvc const, for thread-safety and general cleanliness.
Here, we add change GeoSvc::getDetector and constantAsString to be const. However, for now (until the interface class is changed) we also retain the non-const overloads.

Also prepare for IGeoSvc::getDetector returning a const pointer.

BEGINRELEASENOTES

• Added const versions of the IGeoSvc interfaces to GeoSvc.
  ENDRELEASENOTES

[jmcarcell]

I would prefer to deprecate the current and adding the const version at the same time, then we should find in the stack if there is code that won't compile without the existing functions. Otherwise the functions that are added in this PR are never going to be used, since GeoSvc is almost always if not always used through the interface.

[scott-snyder]

I'd rather not have other people seeing warnings from what i'm doing.

This change is actually step one of three to allow this update
without breaking nightlies in the process. First,
add the new methods to the derived class, so it has both (may need to
also temporarily remove override keywords, to prevent warnings from clang).
It should still compile with the old version of the interface.
Second, update the interface class. The derived class should still compile.
In the case where we're just adding const qualifiers to the methods, then
clients should still compile as well (though some extra checking is needed
if as in the case here a const qualifier also needs to be added to a
return type). Third, remove the old methods from the derived class
and add proper override qualifiers. I have these changes all prepared
(for this class and also a bunch from k4RecCalorimeter), but they need
to be submitted one at a time.

That was the best i could come up with to get around the lack of any way
to do atomic updates...

[jmcarcell]

But we can not simply change it in the interface (your step 3) - it is being used by many packages that we may have to change before removing the non-const functions. In addition we don't know if there is anyone with a repository using the non-const ones, then failing to compile (and possibly wasting our and their time by asking what's going on) is much worse than getting some deprecation warnings with instructions. Then step 1 would be to add them to the interface with warnings for the other ones and step 2 is changing in all possible repositories. Old and new can coexist and CI builds don't care about deprecation warnings. This is how we typically do it, see, for example, key4hep/EDM4hep#315.

[scott-snyder]

hi -

One can of course change the interfaace itself in several steps, first
adding the new method along with a deprecation warning on the old
method, and removing the old method as a separate step. Actually,
i would first add the new method, then go through all repos i know about
and submit PRs to change from old to new, and only once they have
all been merged add the deprecation warning --- otherwise, you're just
dumping your mess in other people's laps for them to clean up.
Then there's the question of how long to leave the old interface
there --- what you don't know, you don't know, so you never know
if it's actually safe to remove it. So you just have to do it after
some arbitrary time and hope for the best.

And deprecation warnings don't really help for the derived classes implementing
the interface --- there need to be implementations of pure virtual methods
there before the declarations are added to the interface class.
One can add a default implementation of the new method, of course,
but going that way seems to be even messier and requires more steps.

And for the case of just making a method const, adding deprecation warnings
doesn't actually help. Eg, if i change

class IWhatever { ...
  virtual void foo() = 0;
}

to

class IWhatever { ...
  virtual void foo() const = 0;
}

then any exiting clients that called foo() via a IWhatever* will still work.
If we kept both methods with the non-const one deprecated, then people
would see warnings which to get rid of they would need to do a unneccessary
cast to IWhatever const*.

The harder case is the other method here, were the return type also changes,

eg from

  virtual Detector* getDetector() = 0;

to

  virtual const Detector* getDetector() const = 0;

Here, if one does

  Detector* d = geoSvc->getDetector();

then one does to change the call. But i've only seen one instance of this;
the vast majority of calls are more like

  dd4hep::DetElement DCH_DE = m_geoSvc->getDetector()->detectors().at(DCH_name);

which will compile fine with both the const and non-const version; adding
a deprecation warning would then require an unneeded cast to silence.
(Though in fairness it only works because the dd4hep interfaces are such
a disaster with respect to constness --- but one thing at a time.)

Regardless, that would be for when the interface class gets changed.
It's not clear to me what you want changed for this PR.
You said

I would prefer to deprecate the current and adding the const version at the same time, then we should find in the stack if there is code that won't compile without the existing functions.

But if we change the interface class before changing the derived class
then we will get compilation failures.

CI builds don't care about deprecation warnings.

Wouldn't that just result in people ignoring/not noticing the deprecations?
(That's been my experience, anyway...)