using getFirstDecl for TranslationUnitDecl for consistent pointers

[Vipul-Cariappa]

Eight previously failing tests pass without any new failing tests in cppyy.

But EnumReflectionTest.GetEnums fails from the CppInterOp tests.
It looks like GetEnums cannot resolve any enums in the global scope.

[Vipul-Cariappa]

ping @vgvassilev @aaronj0
Any ideas on what is causing the CppInterOp failure?

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[vgvassilev]

Looks like some test is failing.

[vgvassilev]

Eight previously failing tests pass without any new failing tests in cppyy.

But EnumReflectionTest.GetEnums fails from the CppInterOp tests. It looks like GetEnums cannot resolve any enums in the global scope.

The problem is that the implementation is insufficient. To fix it, we need to iterate the redeclaration chains of NamespaceDecl and TranslationUnitDecl. That is, we should check if the Decl is Redeclarable and then do getMostRecentDecl() and loop until there is getPreviousDecl()...

[vgvassilev]

Eight previously failing tests pass without any new failing tests in cppyy.
But EnumReflectionTest.GetEnums fails from the CppInterOp tests. It looks like GetEnums cannot resolve any enums in the global scope.

The problem is that the implementation is insufficient. To fix it, we need to iterate the redeclaration chains of NamespaceDecl and TranslationUnitDecl. That is, we should check if the Decl is Redeclarable and then do getMostRecentDecl() and loop until there is getPreviousDecl()...

Even better DeclContext::collectAllContexts does it for us.

[Vipul-Cariappa]

The problem is that the implementation is insufficient. To fix it, we need to iterate the redeclaration chains of NamespaceDecl and TranslationUnitDecl.

Fixed. 👍🏽

---

Maybe there are other places where we will need to use DeclContext::collectAllContexts to resolve symbols.

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[vgvassilev]

The problem is that the implementation is insufficient. To fix it, we need to iterate the redeclaration chains of NamespaceDecl and TranslationUnitDecl.

Fixed. 👍🏽

Maybe there are other places where we will need to use DeclContext::collectAllContexts to resolve symbols.

Hopefully not, because this iteration generally is wrong. This need to be lookup-based approach instead of a brute forcing all namespaces and translation units... Please add a giant FIXME describing that.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.71%. Comparing base (57d09bd) to head (2d2b70c).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #329      +/-   ##
==========================================
+ Coverage   73.69%   73.71%   +0.02%     
==========================================
  Files           8        8              
  Lines        3018     3021       +3     
==========================================
+ Hits         2224     2227       +3     
  Misses        794      794              

Files with missing lines	Coverage Δ
lib/Interpreter/CppInterOp.cpp	79.57% <100.00%> (+0.03%)	⬆️

Files with missing lines	Coverage Δ
lib/Interpreter/CppInterOp.cpp	79.57% <100.00%> (+0.03%)	⬆️

[github-actions]

clang-tidy made some suggestions

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[vgvassilev]

LGTM!

[aaronj0]

@Vipul-Cariappa Can you rebase into a single commit?

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[vgvassilev]

The commit does more than it says?

[Vipul-Cariappa]

The commit does more than it says?

The main purpose of the PR is to use getFirstDecl for TranslationUnitDecl. That breaks GetEnums therefore we had to refactor it to use collectAllContexts.

Hope the new commit message conveys the same.

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"