[RF] Fix debug build with C++14

[hahnjo]

Commit ff86c30 ("[RF] Implement SumW2 correction in new BatchMode with RooFitDriver") introduced some static constexpr. When building with C++14, at least weightVarName requires a declaration because it is odr-used. Provide them for all three variables to avoid undefined references seen in debug builds without compiler optimizations.

[phsft-bot]

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

[hahnjo]

To be honest, I don't understand what the compiler is doing here and what the standard mandates here, maybe @Axel-Naumann or @jalopezg-r00t can enlighten me here.

Another "fix" that also appears to works:

diff --git a/roofit/roofitcore/src/RooNLLVarNew.cxx b/roofit/roofitcore/src/RooNLLVarNew.cxx
index 0a810d1a4b..6922d6151f 100644
--- a/roofit/roofitcore/src/RooNLLVarNew.cxx
+++ b/roofit/roofitcore/src/RooNLLVarNew.cxx
@@ -40,6 +40,8 @@ functions from `RooBatchCompute` library to provide faster computation times.

 using namespace ROOT::Experimental;

+constexpr const char *RooNLLVarNew::weightVarName;
+
 namespace {

 std::unique_ptr<RooAbsReal> createRangeNormTerm(RooAbsPdf const &pdf, RooArgSet const &observables,

Which looks odd for constexpr, to be honest...

[guitargeek]

Hi, thanks for this fix! Good that you found a quick arcane solution 👍

Please also add a line with a comment explaining why make_unique is not used in that case, so that nobody will accidentally change it back and re-introduce the issue.

[hahnjo]

Please also add a line with a comment explaining why make_unique is not used in that case, so that nobody will accidentally change it back and re-introduce the issue.

That doesn't help at all, you can introduce this issue in any place you use these static constexpr. We have to understand constexpr and not add comments in one place where the issue popped up. On top, I'd argue that std::make_unique was wrong here anyway since it's relying on a temporary object + move semantics instead of just setting the address of the already constructed std::unique_ptr.

[jalopezg-git]

To be honest, I don't understand what the compiler is doing here and what the standard mandates here, maybe @Axel-Naumann or @jalopezg-r00t can enlighten me here.

Another "fix" that also appears to works:

diff --git a/roofit/roofitcore/src/RooNLLVarNew.cxx b/roofit/roofitcore/src/RooNLLVarNew.cxx
index 0a810d1a4b..6922d6151f 100644
--- a/roofit/roofitcore/src/RooNLLVarNew.cxx
+++ b/roofit/roofitcore/src/RooNLLVarNew.cxx
@@ -40,6 +40,8 @@ functions from `RooBatchCompute` library to provide faster computation times.

 using namespace ROOT::Experimental;

+constexpr const char *RooNLLVarNew::weightVarName;
+
 namespace {

 std::unique_ptr<RooAbsReal> createRangeNormTerm(RooAbsPdf const &pdf, RooArgSet const &observables,

Which looks odd for constexpr, to be honest...

Even if constexpr static member declarations can be initialized in place, I think the standard mandates also a definition if the name is odr-used (EDIT: required only until C++17). See https://en.cppreference.com/w/cpp/language/static, Section "Constant static members". :-)

[guitargeek]

Please also add a line with a comment explaining why make_unique is not used in that case, so that nobody will accidentally change it back and re-introduce the issue.

That doesn't help at all, you can introduce this issue in any place you use these static constexpr. We have to understand constexpr and not add comments in one place where the issue popped up. On top, I'd argue that std::make_unique was wrong here anyway since it's relying on a temporary object + move semantics instead of just setting the address of the already constructed std::unique_ptr.

Ok then let's use the constexpr correctly here. So with what @jalopezg-r00t said, the correct fix is to also add the definition even if it looks strange? If you go for that, please also add a comment explaining why the line is necessary so it is not accidentally removed because it looks superfluous.

As for the make_unique, I think it's correct to use it there. According to the core guidelines [1], it is always preferred because it "gives a more concise statement of the construction. It also ensures exception safety in complex expressions". And moving a unique pointer is cheap, temporary unique_ptr are not a problem.

[1] https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-make_unique

[hahnjo]

As for the make_unique, I think it's correct to use it there. According to the core guidelines [1], it is always preferred because it "gives a more concise statement of the construction. It also ensures exception safety in complex expressions". And moving a unique pointer is cheap, temporary unique_ptr are not a problem.

This guideline entry talks about construction, ie auto q = make_unique<Foo>(7);. Here we already have weightVar constructed and want to assign to it.

[guitargeek]

I don't see why the same reasons of being more concise and ensuring exception safety should not apply in this case. And move assignment and move construction of unique pointers should be equally cheap, right?

[hahnjo]

There is no issue of exception safety here, it's only a new that can work or not.

And move assignment and move construction of unique pointers should be equally cheap, right?

With compiler optimizations yes, but why create unnecessary work?

[guitargeek]

RooFit is never used without optimization flags, and I want to avoid calling new and delete explicitly in RooFit to minimize the risk of unintended memory leaks.

[phsft-bot]

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

[hahnjo]

Ok then let's use the constexpr correctly here. So with what @jalopezg-r00t said, the correct fix is to also add the definition even if it looks strange? If you go for that, please also add a comment explaining why the line is necessary so it is not accidentally removed because it looks superfluous.

Done.

[phsft-bot]

Build failed on ROOT-ubuntu2004/soversion.
Running on root-ubuntu-2004-3.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

• projectroot.roofit.roofitcore.test.gtest_roofit_roofitcore_test_testRooSimultaneous

[phsft-bot]

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-1.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

• projectroot.roofit.roofitcore.test.gtest_roofit_roofitcore_test_testRooSimultaneous

[phsft-bot]

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

• projectroot.roofit.roofitcore.test.gtest_roofit_roofitcore_test_testRooSimultaneous

[guitargeek]

LGTM, thank you it's perfect now 👍

Can you merge this soon? Maybe don't worry about the backport this time, because the original commit that this PR is a fixup to is not backported yet. I'll backport everything in one go to not have an intermediate state where the debug build is broken.