[RF] Don't allow passing temporaries to RooArgList/Set constructors by guitargeek · Pull Request #8065 · root-project/root

guitargeek · 2021-04-30T16:26:15Z

The two classes inheriting from RooAbsCollection, namely RooArgList and
RooArgSet are by default non-owning collections. This means they should
not be initialized with temporary objects, otherwise they contain
invalid pointers right after construction.

To avoid that this can ever happen, the universal reference mechanism is
used to statically check for temporaries, such that your code doesn't
even compile if you try to construct a RooArgList or RooArgSet from
temporaries.

This fixes #8052.

Some C++ code to validate that the static_assert works:

#include "RooRealVar.h"
#include "RooArgList.h"
#include "RooArgSet.h"
#include "RooUniform.h"

void test() {

    RooRealVar x("x", "x", 0, 0, 10);
    RooRealVar y("y", "y", 0, 0, 10);

    RooUniform u1("u1", "u1", x);
    RooUniform u2("u2", "u2", y);

    // this should work
    RooArgSet (u1, u2);
    RooArgList(u1, u2);

    // all of these combinations should cause compiler errors
    ///RooArgSet (RooUniform("u1", "u1", x), u2                       );
    //RooArgSet (u1,                        RooUniform("u2", "u2", y));
    //RooArgSet (RooUniform("u1", "u1", x), RooUniform("u2", "u2", y));
    //RooArgList(RooUniform("u1", "u1", x), u2                       );
    //RooArgList(u1,                        RooUniform("u2", "u2", y));
    //RooArgList(RooUniform("u1", "u1", x), RooUniform("u2", "u2", y));

}

phsft-bot · 2021-05-26T00:46:47Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-06-29T18:33:21Z

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

phsft-bot · 2021-06-29T18:35:57Z

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

phsft-bot · 2021-06-30T03:35:53Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2021-07-06T10:25:07Z

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

phsft-bot · 2021-07-06T10:25:56Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-07-06T10:25:55.753Z] CMake Error at /home/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1045 (message):

phsft-bot · 2021-07-06T10:26:36Z

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

phsft-bot · 2021-07-06T10:27:06Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-07-06T10:27:05.590Z] CMake Error at /home/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1045 (message):

phsft-bot · 2021-07-06T11:11:46Z

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

Failing tests:

projectroot.hist.histdrawv7.test.gtest_hist_histdrawv7_test_histhistdrawv7testUnit

hageboeck · 2021-07-06T16:18:53Z

roofit/roofitcore/inc/RooArgList.h

+  /// RooAbsArg as r-value reference is not allowed.
+  template<typename... Arg_t>
+  RooArgList(RooAbsArg && arg, Arg_t &&... argsOrName)
+    : RooArgList{arg, std::move(arg), std::forward<Arg_t>(argsOrName)...} {}


Really std::move(arg), not forward?

I would stay std::move, because here the RooAbsArg && arg is not a universal reference but an r-value reference.

I I think the more general std::forward is intended to forward universal references correctly, but here I explicitly create a new r-value reference from arg, which is what move does. Of course they both work, if you want I can change :)

(rebasing and solving conflicts in this PR at the moment)

Oh, so you want temporaries all the way in the arguments? That might work indeed.

The two classes inheriting from RooAbsCollection, namely RooArgList and RooArgSet are by default non-owning collections. This means they should not be initialized with temporary objects, otherwise they contain invalid pointers right after construction. To avoid that this can ever happen, the universal reference mechanism is used to statically check for temporaries, such that your code doesn't even compile if you try to construct a RooArgList or RooArgSet from temporaries.

phsft-bot · 2021-07-06T17:06:40Z

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

phsft-bot · 2021-07-06T17:13:17Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-07-06T17:12:57.365Z] CMake Error at /home/sftnight/build/workspace/root-pullrequests-build/build/TBB-prefix/src/TBB-stamp/TBB-build-Release.cmake:49 (message):

phsft-bot · 2021-07-06T17:47:51Z

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

Failing tests:

projectroot.hist.histdrawv7.test.gtest_hist_histdrawv7_test_histhistdrawv7testUnit

lmoneta

Very nice addition to avoid the problem of temporaries.
I think the PR is good to be merged.

guitargeek added the in:RooFit/RooStats label Apr 30, 2021

guitargeek requested a review from hageboeck as a code owner April 30, 2021 16:26

guitargeek self-assigned this Apr 30, 2021

guitargeek requested a review from lmoneta as a code owner April 30, 2021 16:26

guitargeek force-pushed the issue-8052 branch from 9350bf0 to d0086cd Compare April 30, 2021 16:51

root-project deleted a comment from phsft-bot May 26, 2021

guitargeek force-pushed the issue-8052 branch from d0086cd to 88c8775 Compare May 26, 2021 00:46

guitargeek force-pushed the issue-8052 branch from 88c8775 to 45239d0 Compare June 29, 2021 18:33

guitargeek force-pushed the issue-8052 branch from 45239d0 to 309166d Compare June 29, 2021 18:35

guitargeek force-pushed the issue-8052 branch from 309166d to d7fe0ee Compare July 6, 2021 10:24

guitargeek force-pushed the issue-8052 branch from d7fe0ee to 5c3984b Compare July 6, 2021 10:26

hageboeck reviewed Jul 6, 2021

View reviewed changes

guitargeek force-pushed the issue-8052 branch from 5c3984b to 9f21e3a Compare July 6, 2021 17:06

lmoneta approved these changes Jul 21, 2021

View reviewed changes

guitargeek merged commit 6e5677f into root-project:master Jul 21, 2021

guitargeek deleted the issue-8052 branch July 21, 2021 16:00

Conversation

guitargeek commented Apr 30, 2021

Uh oh!

phsft-bot commented May 26, 2021

Uh oh!

phsft-bot commented Jun 29, 2021

Uh oh!

phsft-bot commented Jun 29, 2021

Uh oh!

phsft-bot commented Jun 30, 2021

Failing tests:

Uh oh!

phsft-bot commented Jul 6, 2021

Uh oh!

phsft-bot commented Jul 6, 2021

Errors:

Uh oh!

phsft-bot commented Jul 6, 2021

Uh oh!

phsft-bot commented Jul 6, 2021

Errors:

Uh oh!

phsft-bot commented Jul 6, 2021

Failing tests:

Uh oh!

hageboeck Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

guitargeek Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

guitargeek Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

hageboeck Jul 9, 2021

Choose a reason for hiding this comment

Uh oh!

phsft-bot commented Jul 6, 2021

Uh oh!

phsft-bot commented Jul 6, 2021

Errors:

Uh oh!

phsft-bot commented Jul 6, 2021

Failing tests:

Uh oh!

lmoneta left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants