diff --git a/core/metacling/src/TClingCallFunc.cxx b/core/metacling/src/TClingCallFunc.cxx index 7387068e938ee..3d76c8f7787ad 100644 --- a/core/metacling/src/TClingCallFunc.cxx +++ b/core/metacling/src/TClingCallFunc.cxx @@ -229,6 +229,30 @@ void TClingCallFunc::collect_type_info(QualType &QT, ostringstream &typedefbuf, GetTypeAsString(QT, type_name, C, Policy); } +static bool IsCopyConstructorDeleted(QualType QT) +{ + CXXRecordDecl *RD = QT->getAsCXXRecordDecl(); + if (!RD) { + // For types that are not C++ records (such as PODs), we assume that they are copyable, ie their copy constructor + // is not deleted. + return false; + } + + RD = RD->getDefinition(); + assert(RD && "expecting a definition"); + + if (RD->hasSimpleCopyConstructor()) + return false; + + for (auto *Ctor : RD->ctors()) { + if (Ctor->isCopyConstructor()) { + return Ctor->isDeleted(); + } + } + + assert(0 && "did not find a copy constructor?"); +} + void TClingCallFunc::make_narg_ctor(const unsigned N, ostringstream &typedefbuf, ostringstream &callbuf, const string &class_name, int indent_level) @@ -240,6 +264,10 @@ void TClingCallFunc::make_narg_ctor(const unsigned N, ostringstream &typedefbuf, const FunctionDecl *FD = GetDecl(); callbuf << "new " << class_name << "("; + + // IsCopyConstructorDeleted could trigger deserialization of decls. + cling::Interpreter::PushTransactionRAII RAII(fInterp); + for (unsigned i = 0U; i < N; ++i) { const ParmVarDecl *PVD = FD->getParamDecl(i); QualType Ty = PVD->getType(); @@ -268,7 +296,17 @@ void TClingCallFunc::make_narg_ctor(const unsigned N, ostringstream &typedefbuf, callbuf << "*(" << type_name.c_str() << "**)args[" << i << "]"; } else { + // By-value construction: Figure out if the type can be copy-constructed. This is tricky and cannot be done in + // a fully reliable way, also because std::vector always defines a copy constructor, even if the type T is + // only moveable. As a heuristic, we only check if the copy constructor is deleted, or would be if implicit. + bool Move = IsCopyConstructorDeleted(QT); + if (Move) { + callbuf << "static_cast<" << type_name << "&&>("; + } callbuf << "*(" << type_name.c_str() << "*)args[" << i << "]"; + if (Move) { + callbuf << ")"; + } } } callbuf << ")"; @@ -354,6 +392,10 @@ void TClingCallFunc::make_narg_call(const std::string &return_type, const unsign if (ShouldCastFunction) callbuf << ")"; callbuf << "("; + + // IsCopyConstructorDeleted could trigger deserialization of decls. + cling::Interpreter::PushTransactionRAII RAII(fInterp); + for (unsigned i = 0U; i < N; ++i) { const ParmVarDecl *PVD = FD->getParamDecl(i); QualType Ty = PVD->getType(); @@ -383,9 +425,17 @@ void TClingCallFunc::make_narg_call(const std::string &return_type, const unsign callbuf << "*(" << type_name.c_str() << "**)args[" << i << "]"; } else { - // pointer falls back to non-pointer case; the argument preserves - // the "pointerness" (i.e. doesn't reference the value). + // By-value construction: Figure out if the type can be copy-constructed. This is tricky and cannot be done in + // a fully reliable way, also because std::vector always defines a copy constructor, even if the type T is + // only moveable. As a heuristic, we only check if the copy constructor is deleted, or would be if implicit. + bool Move = IsCopyConstructorDeleted(QT); + if (Move) { + callbuf << "static_cast<" << type_name << "&&>("; + } callbuf << "*(" << type_name.c_str() << "*)args[" << i << "]"; + if (Move) { + callbuf << ")"; + } } } callbuf << ")"; diff --git a/core/metacling/test/CMakeLists.txt b/core/metacling/test/CMakeLists.txt index 98c58074f61c8..aed9784adcd27 100644 --- a/core/metacling/test/CMakeLists.txt +++ b/core/metacling/test/CMakeLists.txt @@ -31,3 +31,7 @@ ROOT_ADD_GTEST( add_dependencies(TClingTest Cling RIO) ROOT_ADD_GTEST(TClingLoadUnloadFile TClingLoadUnloadFileTests.cxx LIBRARIES Cling RIO) + +if(pyroot) + ROOT_ADD_PYUNITTEST(TClingCallFuncTests TClingCallFuncTests.py) +endif() diff --git a/core/metacling/test/TClingCallFuncTests.cxx b/core/metacling/test/TClingCallFuncTests.cxx index 87e296ab942c3..5bb063fbf0221 100644 --- a/core/metacling/test/TClingCallFuncTests.cxx +++ b/core/metacling/test/TClingCallFuncTests.cxx @@ -324,3 +324,182 @@ TEST(TClingCallFunc, GH_14405) { result = gInterpreter->CallFunc_ExecDouble(cf, /*address=*/0); EXPECT_NEAR(result, 7.28, /*abs_error=*/1e-6); } + +TEST(TClingCallFunc, GH_14425) +{ + gInterpreter->Declare(R"cpp( + struct GH_14425 { + int fMember; + GH_14425(int m = 1) : fMember(m) {} + GH_14425(const GH_14425&) = delete; + GH_14425(GH_14425&&) = default; + }; + int GH_14425_f(GH_14425 p = GH_14425()) { return p.fMember; } + int GH_14425_g(GH_14425 p) { return p.fMember; } + struct GH_14425_Copyable { + int fMember; + GH_14425_Copyable(int m = 1) : fMember(m) {} + GH_14425_Copyable(const GH_14425_Copyable &o) : fMember(o.fMember) {} + GH_14425_Copyable(GH_14425_Copyable &&o) : fMember(o.fMember) { o.fMember = 0; } + }; + int GH_14425_h(GH_14425_Copyable p) { return p.fMember; } + struct GH_14425_Default { + int fMember; + GH_14425_Default(GH_14425 p = GH_14425()) : fMember(p.fMember) {} + }; + struct GH_14425_Required { + int fMember; + GH_14425_Required(GH_14425 p) : fMember(p.fMember) {} + }; + )cpp"); + CallFuncRAII CfDefaultRAII("", "GH_14425_f", ""); + int valDefault = gInterpreter->CallFunc_ExecInt(CfDefaultRAII.GetCF(), /*address*/ 0); + EXPECT_EQ(valDefault, 1); + + CallFuncRAII CfArgumentRAII("", "GH_14425_f", "GH_14425"); + CallFunc_t *CfArgument = CfArgumentRAII.GetCF(); + // Cheat a bit: GH_14425 has only one int fMember in memory... + int objArgument = 2; + gInterpreter->CallFunc_SetArg(CfArgument, &objArgument); + int valArgument = gInterpreter->CallFunc_ExecInt(CfArgument, /*address*/ 0); + EXPECT_EQ(valArgument, 2); + + CallFuncRAII CfRequiredRAII("", "GH_14425_g", "GH_14425"); + CallFunc_t *CfRequired = CfRequiredRAII.GetCF(); + // Cheat a bit: GH_14425 has only one int fMember in memory... + int objRequired = 3; + gInterpreter->CallFunc_SetArg(CfRequired, &objRequired); + int valRequired = gInterpreter->CallFunc_ExecInt(CfRequired, /*address*/ 0); + EXPECT_EQ(valRequired, 3); + + CallFuncRAII CfCopyableRAII("", "GH_14425_h", "GH_14425_Copyable"); + CallFunc_t *CfCopyable = CfCopyableRAII.GetCF(); + // Cheat a bit: GH_14425_Copyable has only one int fMember in memory... + int objCopyable = 4; + gInterpreter->CallFunc_SetArg(CfCopyable, &objCopyable); + int valCopyable = gInterpreter->CallFunc_ExecInt(CfCopyable, /*address*/ 0); + EXPECT_EQ(valCopyable, 4); + // The original value should not have changed; if it did, TClingCallFunc called the move constructor. + EXPECT_EQ(objCopyable, 4); + + CallFuncRAII CfConstructorDefaultRAII("GH_14425_Default", "GH_14425_Default", ""); + int *valConstructorDefault; + gInterpreter->CallFunc_ExecWithReturn(CfConstructorDefaultRAII.GetCF(), /*address*/ 0, &valConstructorDefault); + EXPECT_EQ(*valConstructorDefault, 1); + + CallFuncRAII CfConstructorArgumentRAII("GH_14425_Default", "GH_14425_Default", "GH_14425"); + CallFunc_t *CfConstructorArgument = CfConstructorArgumentRAII.GetCF(); + // Cheat a bit: GH_14425 has only one int fMember in memory... + int objConstructorArgument = 2; + gInterpreter->CallFunc_SetArg(CfConstructorArgument, &objConstructorArgument); + int *valConstructorArgument; + gInterpreter->CallFunc_ExecWithReturn(CfConstructorArgument, /*address*/ 0, &valConstructorArgument); + EXPECT_EQ(*valConstructorArgument, 2); + + CallFuncRAII CfConstructorRequiredRAII("GH_14425_Required", "GH_14425_Required", "GH_14425"); + CallFunc_t *CfConstructorRequired = CfConstructorRequiredRAII.GetCF(); + // Cheat a bit: GH_14425 has only one int fMember in memory... + int objConstructorRequired = 3; + gInterpreter->CallFunc_SetArg(CfConstructorRequired, &objConstructorRequired); + int *valConstructorRequired; + gInterpreter->CallFunc_ExecWithReturn(CfConstructorRequired, /*address*/ 0, &valConstructorRequired); + EXPECT_EQ(*valConstructorRequired, 3); +} + +TEST(TClingCallFunc, GH_14425_Virtual) +{ + // Virtual classes are a bit more complicated, we need to declare them both compiled and in the interpreter. + struct GH_14425_Virtual { + int fMember; + GH_14425_Virtual(int m = 1) : fMember(m) {} + GH_14425_Virtual(const GH_14425_Virtual &) = default; + GH_14425_Virtual(GH_14425_Virtual &&o) : fMember(o.fMember) { o.fMember = 0; } + void f() {} + }; + gInterpreter->Declare(R"cpp( + struct GH_14425_Virtual { + int fMember; + GH_14425_Virtual(int m = 1) : fMember(m) {} + GH_14425_Virtual(const GH_14425_Virtual &) = default; + GH_14425_Virtual(GH_14425_Virtual &&o) : fMember(o.fMember) { o.fMember = 0; } + void f() {} + }; + int GH_14425_v(GH_14425_Virtual p) { return p.fMember; } + )cpp"); + CallFuncRAII CfVirtualRAII("", "GH_14425_v", "GH_14425_Virtual"); + CallFunc_t *CfVirtual = CfVirtualRAII.GetCF(); + GH_14425_Virtual objVirtual(2); + gInterpreter->CallFunc_SetArg(CfVirtual, &objVirtual); + int valVirtual = gInterpreter->CallFunc_ExecInt(CfVirtual, /*address*/ 0); + EXPECT_EQ(valVirtual, 2); + // The original value should not have changed; if it did, TClingCallFunc called the move constructor. + EXPECT_EQ(objVirtual.fMember, 2); +} + +TEST(TClingCallFunc, GH_14425_Templates) +{ + // While according to the C++ standard, GH_14425_Moveable has no move-constructor (which must not be a template), + // it has a template constructor that can be instantiated to the move-constructor signature. MSVC uses this for their + // implementation of std::unique_ptr. + gInterpreter->Declare(R"cpp( + struct GH_14425_Moveable { + int fMember = 0; + GH_14425_Moveable(int m = 1) : fMember(m) {}; + GH_14425_Moveable(const GH_14425_Moveable&) = delete; + template + GH_14425_Moveable(T &&t) : fMember(t.fMember) {} + }; + int GH_14425_Moveable_f(GH_14425_Moveable p) { return p.fMember; } + struct GH_14425_Moveable_Required { + int fMember; + GH_14425_Moveable_Required(GH_14425_Moveable p) : fMember(p.fMember) {} + }; + template struct GH_14425_T { + T fMember = 0; + GH_14425_T(T m = 1) : fMember(m) {} + GH_14425_T(const GH_14425_T&) = delete; + GH_14425_T(GH_14425_T &&t) = default; + }; + template + T GH_14425_t(GH_14425_T p) { return p.fMember; } + template struct GH_14425_I { + std::unique_ptr fMember; + GH_14425_I(std::unique_ptr m) : fMember(std::move(m)) {} + }; + template + T GH_14425_i(GH_14425_I p) { return *p.fMember; } + )cpp"); + CallFuncRAII CfMoveableRAII("", "GH_14425_Moveable_f", "GH_14425_Moveable"); + CallFunc_t *CfMoveable = CfMoveableRAII.GetCF(); + // Cheat a bit: GH_14425_Moveable has only one int fMember in memory... + int objMoveable = 4; + gInterpreter->CallFunc_SetArg(CfMoveable, &objMoveable); + int valMoveable = gInterpreter->CallFunc_ExecInt(CfMoveable, /*address*/ 0); + EXPECT_EQ(valMoveable, 4); + + CallFuncRAII CfConstructorRequiredRAII("GH_14425_Moveable_Required", "GH_14425_Moveable_Required", + "GH_14425_Moveable"); + CallFunc_t *CfConstructorRequired = CfConstructorRequiredRAII.GetCF(); + // Cheat a bit: GH_14425_Moveable has only one int fMember in memory... + int objConstructorRequired = 5; + gInterpreter->CallFunc_SetArg(CfConstructorRequired, &objConstructorRequired); + int *valConstructor; + gInterpreter->CallFunc_ExecWithReturn(CfConstructorRequired, /*address*/ 0, &valConstructor); + EXPECT_EQ(*valConstructor, 5); + + CallFuncRAII CfTRAII("", "GH_14425_t", "GH_14425_T"); + CallFunc_t *CfT = CfTRAII.GetCF(); + // Cheat a bit: GH_14425_T has only one int fMember in memory... + int objT = 6; + gInterpreter->CallFunc_SetArg(CfT, &objT); + int valT = gInterpreter->CallFunc_ExecInt(CfT, /*address*/ 0); + EXPECT_EQ(valT, 6); + + CallFuncRAII CfIRAII("", "GH_14425_i", "GH_14425_I"); + CallFunc_t *CfI = CfIRAII.GetCF(); + // Cheat a bit: GH_14425_I has only one std::unique_ptr fMember in memory... + auto objI = std::make_unique(7); + gInterpreter->CallFunc_SetArg(CfI, &objI); + int valI = gInterpreter->CallFunc_ExecInt(CfI, /*address*/ 0); + EXPECT_EQ(valI, 7); +} diff --git a/core/metacling/test/TClingCallFuncTests.py b/core/metacling/test/TClingCallFuncTests.py new file mode 100644 index 0000000000000..fa269103cb07a --- /dev/null +++ b/core/metacling/test/TClingCallFuncTests.py @@ -0,0 +1,34 @@ +import unittest + +import ROOT + +class TClingCallFuncTest(unittest.TestCase): + """Tests related to TClingCallFunc usage from Python""" + + def test_GH_14425(self): + """Can call a function with non-copyable argument.""" + + ROOT.gInterpreter.Declare(r""" +struct GH_14425 { + int fMember; + GH_14425(int m = 1) : fMember(m) {} + GH_14425(const GH_14425&) = delete; + GH_14425(GH_14425&&) = default; +}; +int GH_14425_f(GH_14425 p = GH_14425()) { return p.fMember; } +int GH_14425_g(GH_14425 p) { return p.fMember; } +struct GH_14425_Default { + int fMember; + GH_14425_Default(GH_14425 p = GH_14425()) : fMember(p.fMember) {} +}; +struct GH_14425_Required { + int fMember; + GH_14425_Required(GH_14425 p) : fMember(p.fMember) {} +}; +""") + self.assertEqual(ROOT.GH_14425_f(), 1) + self.assertEqual(ROOT.GH_14425_f(ROOT.GH_14425(2)), 2) + self.assertEqual(ROOT.GH_14425_g(ROOT.GH_14425(3)), 3) + self.assertEqual(ROOT.GH_14425_Default().fMember, 1) + self.assertEqual(ROOT.GH_14425_Default(ROOT.GH_14425(2)).fMember, 2) + self.assertEqual(ROOT.GH_14425_Required(ROOT.GH_14425(3)).fMember, 3) diff --git a/tree/ntuple/v7/test/CMakeLists.txt b/tree/ntuple/v7/test/CMakeLists.txt index a68b3464f0a8d..de7dcbfbe87e4 100644 --- a/tree/ntuple/v7/test/CMakeLists.txt +++ b/tree/ntuple/v7/test/CMakeLists.txt @@ -136,5 +136,6 @@ endif() # RNTuple Python interface tests if(pyroot) + ROOT_ADD_PYUNITTEST(ntuple_py_basics ntuple_basics.py) ROOT_ADD_PYUNITTEST(ntuple_py_model ntuple_model.py) endif() diff --git a/tree/ntuple/v7/test/ntuple_basics.py b/tree/ntuple/v7/test/ntuple_basics.py new file mode 100644 index 0000000000000..fd3b16c410415 --- /dev/null +++ b/tree/ntuple/v7/test/ntuple_basics.py @@ -0,0 +1,22 @@ +import unittest + +import ROOT + +RNTupleModel = ROOT.Experimental.RNTupleModel +RNTupleReader = ROOT.Experimental.RNTupleReader +RNTupleWriter = ROOT.Experimental.RNTupleWriter + +class RNTupleBasics(unittest.TestCase): + """Basic tests of using RNTuple from Python""" + + def test_write_read(self): + """Can write and read a basic RNTuple.""" + + model = RNTupleModel.Create() + model.MakeField["int"]("f") + writer = RNTupleWriter.Recreate(model, "ntpl", "test_ntuple_py_write_read.root") + writer.Fill() + del writer + + reader = RNTupleReader.Open("ntpl", "test_ntuple_py_write_read.root") + self.assertEqual(reader.GetNEntries(), 1)