Migrate GlobalChi2Fitter to in-class Gaudi properties

As far as I know, using in-class Gaudi properties is the preferred and modern way of declaring properties for a tool. The GlobalChi2Fitter is currently still using the older in-constructor style properties, and this commit changes this to the new style. Besides being more modern, idiomatic Gaudi, it also means the properties, their config-level names, and their default values are now in a single place is which a lot easier to wrap the head around. The only property not migrated was a service handle, because of some runtime parameters being passed to its declaration statement.

WARNING: big files (>100K) are found in the changeset

284K in file Tracking/TrkFitter/TrkGlobalChi2Fitter/src/GlobalChi2Fitter.cxx

This merge request affects 1 package:

• Tracking/TrkFitter/TrkGlobalChi2Fitter

Adding @amorley as watcher

added Tracking master review-pending-level-1 labels

resolved all threads

CI Result FAILURE (hash ca2a26e3)

	Athena	AthSimulation	AthGeneration	AnalysisBase
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 18600]

The following tests failed:

DataQuality_required-test
Trigger_athena_data-test
Trigger_athena_MC-test

The last two appear in other MRs as well what about the other? Can you have a look?

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

Jenkins please retry a build

WARNING: big files (>100K) are found in the changeset

284K in file Tracking/TrkFitter/TrkGlobalChi2Fitter/src/GlobalChi2Fitter.cxx

This merge request affects 1 package:

• Tracking/TrkFitter/TrkGlobalChi2Fitter

Adding @amorley as watcher

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

CI Result FAILURE (hash ca2a26e3)

	Athena	AthSimulation	AthGeneration	AnalysisBase
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 18649]

This time only the two trigger tests failed - the DQ one ran - it took 25 minutes of time. So it seems like the 30-minute limit is tight!

Too bad this new method loads up header files with so many more includes - which adds to our compile times in other modules!

I'm pushing this to L2 just to have them double-check the modern handling of something like m_trackingGeometrySvc - which requires parameters from the ctor. Otherwise I'd have just approved this! (L1 Shifter)

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

Hi @gwatts, if the increase in compile time is a problem then I am more than happy to close this MR and we can keep the current implementation. I'm not precisely sure what the benefits of the new approach are, only that it looks a bit neater and that it is supposedly the more modern and idiomatic way to do things. But perhaps that trade-off is not worth making here?

resolved all threads

The new approach is indeed cleaner. I would think that the extra includes would not slow the compilation since the code will only be include once (due to #ifndef/#endif), but maybe that is naive. The changes in this MR look fine, so I will mark approved. ~Joe (L2)

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

resolved all threads