Add missing include to FitWithRooFit.h

[iarspider]

PR description:

All RooFit functions now return an owning pointer (std::unique_ptr) instead of a raw pointer. A destructor of std::unique_ptr calls wrapped class' destructor, so that class needs to be fully defined, or the compilation will fail with

/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc11/external/gcc/11.4.1-30ebdc301ebd200f2ae0e3d880258e65/include/c++/11.4.1/bits/unique_ptr.h:83:23: error: invalid application of 'sizeof' to incomplete type 'RooFitResult'
   83 |         static_assert(sizeof(_Tp)>0,
      |                       ^~~~~~~~~~~
gmake: *** [tmp/el8_amd64_gcc11/src/DQMOffline/Alignment/src/DQMOfflineAlignment/DiMuonMassBiasClient.cc.o] Error 1

PR validation:

Tested in cms-sw/cmsdist#8734

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

Before submitting your pull requests, make sure you followed this checklist:

• verify that the PR is really intended for the chosen branch
• verify that changes follow CMS Naming, Coding, And Style Rules
• verify that the PR passes the basic test procedure suggested in the CMSSW PR instructions

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42899/37045

• This PR adds an extra 12KB to repository

[smuzaffar]

please test

[smuzaffar]

ping bot

[cmsbuild]

A new Pull Request was created by @iarspider for master.

It involves the following packages:

• Alignment/OfflineValidation (alca)

@perrotta, @consuegs, @saumyaphor4252 can you please review it and eventually sign? Thanks.
@mmusich, @adewit, @tlampen, @tocheng this is something you requested to watch as well.
@antoniovilela, @sextonkennedy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1c3370/34953/summary.html
COMMIT: 699913c
CMSSW: CMSSW_13_3_X_2023-09-28-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42899/34953/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• You potentially added 5 lines to the logs
• Reco comparison results: 2 differences found in the comparisons
• DQMHistoTests: Total files compared: 50
• DQMHistoTests: Total histograms compared: 3358320
• DQMHistoTests: Total failures: 3
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3358295
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
• Checked 214 log files, 167 edm output root files, 50 DQM output files
• TriggerResults: no differences found

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42899/37054

• This PR adds an extra 20KB to repository

• There are other open Pull requests which might conflict with changes you have proposed:

  • File Alignment/OfflineValidation/interface/FitWithRooFit.h modified in PR(s): Update FitWithRooFit in Alignment/OfflineValidation to fix memory leaks #42900

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1c3370/34984/summary.html
COMMIT: d107100
CMSSW: CMSSW_13_3_X_2023-09-29-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42899/34984/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• You potentially removed 3 lines from the logs
• Reco comparison results: 0 differences found in the comparisons
• DQMHistoTests: Total files compared: 50
• DQMHistoTests: Total histograms compared: 3358320
• DQMHistoTests: Total failures: 0
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3358298
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
• Checked 214 log files, 167 edm output root files, 50 DQM output files
• TriggerResults: no differences found

[iarspider]

@cms-sw/alca-l2 @cms-sw/dqm-l2 could you please review? The change is purely technical.

[perrotta]

@iarspider maybe you can remove the [TEST] flag from the title, if you want it reviewed.

In the FitWithRooFit class the call to RooFitResult is in a commented out line (which would be a dead branch even if uncommented) in the implementation file. My questions:

• why to add the include in the header file of the class rather than in the implementation file?
• why to add it at all?

In GenericTnPFitter.h I don't even see why this include is missing: if it is really needed, could you please explain it in the PR description?

[iarspider]

@perrotta thanks for the review.

@iarspider maybe you can remove the [TEST] flag from the title, if you want it reviewed.
My bad, done.

In the FitWithRooFit class the call to RooFitResult is in a commented out line (which would be a dead branch even if uncommented) in the implementation file. My questions:

* why to add the include in the header file of the class rather than in the implementation file?

No patricular reason, I can test if it will still work when includes are inside implementation file

* why to add it at all?

In GenericTnPFitter.h I don't even see why this include is missing: if it is really needed, could you please explain it in the PR description?

ROOT changed all RooFit function to return owning pointer (std::unique_ptr), so calling code now needs to know about wrapped type, even if it doesn't use the result.

[perrotta]

ROOT changed all RooFit function to return owning pointer (std::unique_ptr), so calling code now needs to know about wrapped type, even if it doesn't use the result.

Uhm, not sure I understand where RooFitResult is wrapped in these codes, but I imagine you verified it somehow.
For me it's fine, no need to move the header include to the implementation file, then.
Just please add some justification of the fix in the PR descritpion field, so that some documentation can remain of it.

[iarspider]

@perrotta done.

[perrotta]

+alca

• Thank you @iarspider

[tjavaid]

+1

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @rappoccio, @antoniovilela, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2)

[antoniovilela]

+1