Skip to content

Add required cling transactions when performing named lookups#853

Open
aaronj0 wants to merge 2 commits intocompiler-research:mainfrom
aaronj0:moreclingraii
Open

Add required cling transactions when performing named lookups#853
aaronj0 wants to merge 2 commits intocompiler-research:mainfrom
aaronj0:moreclingraii

Conversation

@aaronj0
Copy link
Copy Markdown
Collaborator

@aaronj0 aaronj0 commented Mar 13, 2026

From the ROOT migration
Also folds usages of DeclContext::buildLookup into utils::Lookup::Named to avoid repetition

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.78%. Comparing base (22812bc) to head (bf07bcf).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #853      +/-   ##
==========================================
+ Coverage   79.75%   79.78%   +0.03%     
==========================================
  Files          11       11              
  Lines        4031     4037       +6     
==========================================
+ Hits         3215     3221       +6     
  Misses        816      816              
Files with missing lines Coverage Δ
lib/CppInterOp/CppInterOp.cpp 88.01% <100.00%> (+0.03%) ⬆️
lib/CppInterOp/CppInterOpInterpreter.h 84.48% <ø> (ø)
Files with missing lines Coverage Δ
lib/CppInterOp/CppInterOp.cpp 88.01% <100.00%> (+0.03%) ⬆️
lib/CppInterOp/CppInterOpInterpreter.h 84.48% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Collaborator

@Vipul-Cariappa Vipul-Cariappa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Provided all the tests pass without any regression (the cppyy tests are failing right now...).

Comment thread lib/CppInterOp/CppInterOpInterpreter.h Outdated
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

inline void Named(clang::Sema* S, clang::LookupResult& R,
const clang::DeclContext* Within = nullptr) {
if (Within)
Within->getPrimaryContext()->buildLookup();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: 'this' argument to member function 'buildLookup' has type 'const DeclContext', but function is not marked const [clang-diagnostic-error]

    Within->getPrimaryContext()->buildLookup();
    ^
Additional context

llvm/include/clang/AST/DeclBase.h:2679: 'buildLookup' declared here

  StoredDeclsMap *buildLookup();
                  ^

@aaronj0 aaronj0 force-pushed the moreclingraii branch 2 times, most recently from acbee94 to d00028f Compare March 26, 2026 13:08
Copy link
Copy Markdown
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

aaronj0 and others added 2 commits April 23, 2026 13:20
In GetBaseClassOffset, GetVariableOffset, GetFunctionsUsingName, ExistsFunctionTemplate, GetClassTemplatedMethods, LookupDatamember

also fold usages of `DeclContext::buildLookup` into `utils::Lookup::Named` to avoid repetition
Co-authored-by: Vassil Vassilev <v.g.vassilev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants