[WIP] Improved error handling for template instantiation #12449
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1088,9 +1088,11 @@ char* Cppyy::CallS( | |
| cstr = cppstring_to_cstring(*cppresult); | ||
| *length = cppresult->size(); | ||
| cppresult->std::string::~basic_string(); | ||
| free((void *)cppresult); | ||
| } else { | ||
| *length = 0; | ||
| free((void *)cppresult); | ||
| } | ||
| return cstr; | ||
| } | ||
|
|
||
|
|
@@ -2101,55 +2103,80 @@ bool template_compare(std::string n1, std::string n2) { | |
| return n2.compare(0, n1.size(), n1) == 0; | ||
| } | ||
|
|
||
| Cppyy::TCppMethod_t Cppyy::GetMethodTemplate(TCppScope_t scope, const std::string &name, const std::string &proto, | ||
| std::ostringstream &diagnostics) | ||
| { | ||
| // There is currently no clean way of extracting a templated method out of ROOT/meta | ||
| // for a variety of reasons, none of them fundamental. The game played below is to | ||
| // first get any pre-existing functions already managed by ROOT/meta, but if that fails, | ||
| // to do an explicit lookup that ignores the prototype (i.e. the full name should be | ||
| // enough), and finally to ignore the template arguments part of the name as this fails | ||
| // in cling if there are default parameters. | ||
|
|
||
| // It would be possible to get the prototype from the created functions and use that to | ||
| // do a new lookup, after which ROOT/meta will manage the function. However, neither | ||
| // TFunction::GetPrototype() nor TFunction::GetSignature() is of the proper form, so | ||
| // we'll/ manage the new TFunctions instead and will assume that they are cached on the | ||
| // calling side to prevent multiple creations. | ||
|
|
||
| // redirect diagnostics, taking the lock to make sure no other calls pollute the results | ||
| R__WRITE_LOCKGUARD(ROOT::gCoreMutex); | ||
|
|
||
| const std::string diagnosticsold = diagnostics.str(); | ||
| TInterpreter::RedirectDiagnostics redirectRAII(gInterpreter, diagnostics, /*enableColors*/ true, /*indent*/ 4); | ||
|
Comment on lines
+2123
to
+2126
Member
There was a problem hiding this comment. I wonder how this interacts with the future rebase of clingwrapper on top of CppInterop @aaronj0 @Vipul-Cariappa . Do we have similar infrastructure for retrieving interpreter diagnostics without leaking implementation details here?
Contributor
There was a problem hiding this comment. We currently do not have a way of doing this. But it should be relatively easy to implement. The question of "without leaking implementation details", not sure about that. If the question is: Can we propagate why a template instantiation fails? Yes, we propagate the reason.
Member
There was a problem hiding this comment. Maybe a clearer rephrasing of my question would be "can we obtain the same interpreter diagnostics in clingwrapper.cxx, without using any ROOT facility (i.e.
Contributor
There was a problem hiding this comment.
The way I see it, we will still need the gCore mutex for the clingwrapper API to continue supporting ROOT threading model. Which is not a problem since we still link against Core in the design that migrates to InterOp.
As Vipul mentioned, obtaining the diagnostic itself is not yet supported, but it can indeed be done generically, i.e., based on Clang's diagnostics engine (like this PR does). Based on that, I would comment that the right place for the very nice One way to proceed is to keep the current approach and, as part of the migration, upstream the diagnostics from TCling to CppInterOp, and revert the changes in TInterpreter, since we can then interact with the |
||
|
|
||
| TFunction *func = nullptr; | ||
| ClassInfo_t *cl = nullptr; | ||
| if (scope == (TCppScope_t)GLOBAL_HANDLE) { | ||
| func = gROOT->GetGlobalFunctionWithPrototype(name.c_str(), proto.c_str()); | ||
| if (func && name.back() == '>') { | ||
| // make sure that all template parameters match (more are okay, e.g. defaults or | ||
| // ones derived from the arguments or variadic templates) | ||
| if (!template_compare(name, func->GetName())) | ||
| func = nullptr; // happens if implicit conversion matches the overload | ||
| } | ||
| } else { | ||
| TClassRef &cr = type_from_handle(scope); | ||
| if (cr.GetClass()) { | ||
| func = cr->GetMethodWithPrototype(name.c_str(), proto.c_str()); | ||
| if (!func) { | ||
| cl = cr->GetClassInfo(); | ||
| // try base classes to cover a common 'using' case (TODO: this is stupid and misses | ||
| // out on base classes; fix that with improved access to Cling) | ||
| TCppIndex_t nbases = GetNumBases(scope); | ||
| for (TCppIndex_t i = 0; i < nbases; ++i) { | ||
| TClassRef &base = type_from_handle(GetScope(GetBaseName(scope, i))); | ||
| if (base.GetClass()) { | ||
| func = base->GetMethodWithPrototype(name.c_str(), proto.c_str()); | ||
| if (func) | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!func && name.back() == '>' && (cl || scope == (TCppScope_t)GLOBAL_HANDLE)) { | ||
| // try again, ignoring proto in case full name is complete template | ||
| std::ostringstream diagnostics2; | ||
| TInterpreter::RedirectDiagnostics redirectRAII2(gInterpreter, diagnostics2, /*enableColors*/ true, /*indent*/ 4); | ||
|
|
||
| auto declid = gInterpreter->GetFunction(cl, name.c_str()); | ||
| if (declid) { | ||
| auto existing = gMethodTemplates.find(declid); | ||
| if (existing == gMethodTemplates.end()) { | ||
| auto cw = new_CallWrapper(declid, name); | ||
| existing = gMethodTemplates.insert(std::make_pair(declid, cw)).first; | ||
| } | ||
| // replace diagnostics to suppress spurious errors or warnings from the previous | ||
| // failed lookup | ||
| diagnostics = std::ostringstream(); | ||
| diagnostics << diagnosticsold; | ||
| diagnostics << diagnostics2.str(); | ||
|
|
||
| return (TCppMethod_t)existing->second; | ||
| } | ||
| diagnostics << diagnostics2.str(); | ||
| } | ||
|
|
||
| if (func) { | ||
|
|
@@ -2165,15 +2192,24 @@ Cppyy::TCppMethod_t Cppyy::GetMethodTemplate( | |
| if (name.back() == '>') { | ||
| auto pos = name.find('<'); | ||
| if (pos != std::string::npos) { | ||
| std::ostringstream diagnostics2; | ||
| TCppMethod_t cppmeth = GetMethodTemplate(scope, name.substr(0, pos), proto, diagnostics2); | ||
| if (cppmeth) { | ||
| // allow if requested template names match up to the result | ||
| const std::string& alt = GetMethodFullName(cppmeth); | ||
| if (name.size() < alt.size() && alt.find('<') == pos) { | ||
| if (template_compare(name, alt)){ | ||
| // replace diagnostics to suppress spurious errors or warnings | ||
| // from the previous failed lookup | ||
| diagnostics = std::ostringstream(); | ||
| diagnostics << diagnosticsold; | ||
| diagnostics << diagnostics2.str(); | ||
| return cppmeth; | ||
| } | ||
|
|
||
| } | ||
| } | ||
| diagnostics << diagnostics2.str(); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -2636,5 +2672,3 @@ long long Cppyy::GetEnumDataValue(TCppEnum_t etype, TCppIndex_t idata) | |
| TEnumConstant* ecst = (TEnumConstant*)((TEnum*)etype)->GetConstants()->At((int)idata); | ||
| return (long long)ecst->GetValue(); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.