Repository navigation
Conversation
…pture their diagnostics When the best match is a not-yet-instantiated function template specialization, instantiate its body and reject the match if the body is ill-formed. Callers use BestOverloadFunctionMatch to probe whether a valid instantiation exists, so an ill-formed body should surface as "no viable function" rather than as a hard error when a call wrapper is compiled for it later. This mirrors cling's LookupHelper::overloadFunctionSelector. The match is rejected if any error occurred during the instantiation (using a DiagnosticErrorTrap), not only if the specialization itself was marked invalid: errors such as a failing static_assert or an error in a nested instantiation leave the specialization valid, and accepting it would either run code which should not have compiled, or fail at link time for the missing nested definition. The rejected specialization is marked invalid, so that later lookups reject it as well, and InstantiateTemplate no longer returns a specialization marked invalid. The diagnostics from instantiating the body are rendered into a string as clang would print them (with source snippets and instantiation notes) instead of being printed, and are available through the new GetLastOverloadMatchDiagnostics, so that callers can report them where appropriate (e.g. the errors when no valid match is found, or warnings from a successful instantiation), while probing for a valid match stays quiet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1179 +/- ##
==========================================
+ Coverage 88.86% 88.88% +0.02%
==========================================
Files 24 24
Lines 7014 7047 +33
==========================================
+ Hits 6233 6264 +31
- Misses 781 783 +2
🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// of passing them to the installed consumer. Like clangSilent this keeps | ||
| /// probing operations quiet, but the diagnostics remain available to report | ||
| /// where appropriate. | ||
| class clangCapture { |
There was a problem hiding this comment.
warning: class 'clangCapture' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
ropriate.
^|
|
||
| // Diagnostics from the instantiation of the body of the best match in the last | ||
| // call to BestOverloadFunctionMatch on this thread | ||
| static thread_local std::string LastOverloadMatchDiagnostics; |
There was a problem hiding this comment.
warning: variable 'LastOverloadMatchDiagnostics' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
atch on this thread
^| )"; | ||
| GetAllTopLevelDecls(code, Decls); | ||
|
|
||
| auto Candidates = [&Decls](llvm::StringRef name) { |
There was a problem hiding this comment.
warning: no header providing "llvm::StringRef" is directly included [misc-include-cleaner]
unittests/CppInterOp/FunctionReflectionTest.cpp:14:
- #include <llvm/Support/Casting.h>
+ #include <llvm/ADT/StringRef.h>
+ #include <llvm/Support/Casting.h>| auto Candidates = [&Decls](llvm::StringRef name) { | ||
| std::vector<Cpp::FuncRef> candidates; | ||
| for (auto* D : Decls) | ||
| if (auto* FTD = llvm::dyn_cast<FunctionTemplateDecl>(D); |
There was a problem hiding this comment.
warning: no header providing "clang::FunctionTemplateDecl" is directly included [misc-include-cleaner]
unittests/CppInterOp/FunctionReflectionTest.cpp:12:
- #include <cstdint>
+ #include <clang/AST/DeclTemplate.h>
+ #include <cstdint>…s, address clang-tidy Enable -Wdeprecated-declarations explicitly around the declarations whose instantiation is expected to warn, since cling disables it by default on Windows (where the test failed). Delete the copy and move operations of clangCapture, mark the thread-local diagnostics string as an intended non-const global, and include the headers used directly by the test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The three The test's The fix in compiler-research/cppjit is one line in the test: namespace OperatorAddTest {
template <class V>
class CustomVec {
+ template <class> friend class CustomVec;
V fX;It's valid independently of this PR, since it only grants the access the code already relies on. ROOT's vendored copy of the test has the same fix in root-project/root#24053. I haven't opened a cppjit PR yet. The other failures don't come from this PR: the two |
|
clang-tidy review says "All clean, LGTM! 👍" |
Description
When the best match found by
BestOverloadFunctionMatchis a function template specialization whose body hasn't been instantiated yet, this instantiates the body and rejects the match if the body is ill-formed. The diagnostics from that instantiation are captured instead of printed, and made available through a new API,GetLastOverloadMatchDiagnostics().Motivation: in ROOT's Python bindings (cppjit, built on CppInterOp since ROOT 6.42), calling a function template with arguments whose instantiation fails currently either:
static_assertleaves the specialization valid), orWith this change, the caller gets "no viable function" plus the compiler errors to report. Warnings from a successful instantiation can be reported too. This is the CppInterOp part of a long-standing ROOT effort, see root-project/root#12449.
Details:
DiagnosticErrorTrap), not only if the specialization was marked invalid. Errors such as a failingstatic_assertor an error in a nested instantiation leave the specialization valid.InstantiateTemplateno longer returns a specialization marked invalid.DefinitionRequiredstays false: a pattern without a body (defined in a library) is still a valid match.Relation to ROOT's vendored CppInterOp: ROOT carries the first part of this (rejecting ill-formed bodies, with diagnostics suppressed) as
[cppinterop] Reject ill-formed instantiations in BestOverloadFunctionMatch [upstream]on theROOT-cppjit-*branches. This PR is that patch rebased onto the reworkedBestOverloadFunctionMatchfrom #1147, plus capturing the diagnostics instead of discarding them.Open design question: there's now the
Result<T>/ErrorReferror model with drained diagnostics (#1034). Returning the diagnostics through it (e.g. as an overload/deduction payload, onceBestOverloadFunctionMatchmigrates) would fit better than a separate thread-local string. I kept a minimal string API here since the structured diagnostics don't carry clang's rendered snippets and instantiation notes, which are what's most useful to show a user. I'm happy to rework it if you prefer a different approach.Fixes # (none)
Type of change
Testing
Added
FunctionReflection_BestOverloadFunctionMatchIllFormedBody. It covers:static_assert: rejected, error reported, still rejected on a second lookup and byInstantiateTemplate;Built against LLVM 23.1.6 (apt.llvm.org packages, Ubuntu 24.04) and ran
check-cppinterop:CppInterOpTests: 522 passed. The only 2 failures (Interpreter_DetectSystemCompilerIncludePaths, in- and out-of-process) also fail on unmodifiedmainin the same environment.DynamicLibraryManagerTestsandTracingTestspass.The equivalent change on ROOT's vendored copy, together with its use in ROOT's cppjit bindings, is in root-project/root#24053. There, all 65 cppjit/pyroot tests pass, including new Python tests for the reported diagnostics.
Checklist
🤖 Generated with Claude Code