Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidate the template instantiation logic. #128

Conversation

vgvassilev
Copy link
Contributor

This patch offers a single interface for instantiation of class, function and variable templates. That would simplify user code where we do not need to care what is the underlying template pattern (most of the time).

@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.24%. Comparing base (23973b8) to head (0d69570).

❗ Current head 0d69570 differs from pull request most recent head 2aa6cec. Consider uploading reports for the commit 2aa6cec to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #128       +/-   ##
===========================================
- Coverage   78.74%   63.24%   -15.51%     
===========================================
  Files           8       15        +7     
  Lines        3091     4097     +1006     
===========================================
+ Hits         2434     2591      +157     
- Misses        657     1506      +849     
Files Coverage Δ
include/clang/Interpreter/CppInterOp.h 96.29% <ø> (-3.71%) ⬇️
lib/Interpreter/CppInterOp.cpp 73.36% <100.00%> (-13.00%) ⬇️
unittests/CppInterOp/ScopeReflectionTest.cpp 100.00% <100.00%> (ø)

... and 15 files with indirect coverage changes

Files Coverage Δ
include/clang/Interpreter/CppInterOp.h 96.29% <ø> (-3.71%) ⬇️
lib/Interpreter/CppInterOp.cpp 73.36% <100.00%> (-13.00%) ⬇️
unittests/CppInterOp/ScopeReflectionTest.cpp 100.00% <100.00%> (ø)

... and 15 files with indirect coverage changes

@vgvassilev vgvassilev force-pushed the consolidate-template-instantiation branch from cbacdb4 to 0d69570 Compare August 9, 2023 17:59
@aaronj0
Copy link
Collaborator

aaronj0 commented Apr 4, 2024

This looks good to go for the incoming template fix patch in

@vgvassilev vgvassilev force-pushed the consolidate-template-instantiation branch 2 times, most recently from 3280562 to 81bde3d Compare April 4, 2024 14:58
Copy link
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

Comment on lines 2787 to 2795
} else if (auto* VarTemplate = dyn_cast<VarTemplateDecl>(TemplateD)) {
DeclResult R = S.CheckVarTemplateId(VarTemplate, fakeLoc, fakeLoc, TLI);
if (R.isInvalid()) {
// FIXME: Diagnose
}
return R.get();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use 'else' after 'return' [llvm-else-after-return]

Suggested change
} else if (auto* VarTemplate = dyn_cast<VarTemplateDecl>(TemplateD)) {
DeclResult R = S.CheckVarTemplateId(VarTemplate, fakeLoc, fakeLoc, TLI);
if (R.isInvalid()) {
// FIXME: Diagnose
}
return R.get();
}
} if (auto* VarTemplate = dyn_cast<VarTemplateDecl>(TemplateD)) {
DeclResult R = S.CheckVarTemplateId(VarTemplate, fakeLoc, fakeLoc, TLI);
if (R.isInvalid()) {
// FIXME: Diagnose
}
return R.get();
}

@vgvassilev vgvassilev force-pushed the consolidate-template-instantiation branch from 81bde3d to a3750e9 Compare April 4, 2024 15:45
@compiler-research compiler-research deleted a comment from github-actions bot Apr 4, 2024
@compiler-research compiler-research deleted a comment from github-actions bot Apr 4, 2024
@compiler-research compiler-research deleted a comment from github-actions bot Apr 4, 2024
@compiler-research compiler-research deleted a comment from github-actions bot Apr 4, 2024
@compiler-research compiler-research deleted a comment from github-actions bot Apr 4, 2024
This patch offers a single interface for instantiation of class, function and
variable templates. That would simplify user code where we do not need to care
what is the underlying template pattern (most of the time).
@vgvassilev vgvassilev force-pushed the consolidate-template-instantiation branch from 8a31adc to 2aa6cec Compare April 4, 2024 15:51
Copy link
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

}

if (auto* VarTemplate = dyn_cast<VarTemplateDecl>(TemplateD)) {
DeclResult R = S.CheckVarTemplateId(VarTemplate, fakeLoc, fakeLoc, TLI);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'R' is not initialized [cppcoreguidelines-init-variables]

Suggested change
DeclResult R = S.CheckVarTemplateId(VarTemplate, fakeLoc, fakeLoc, TLI);
DeclResult R = 0 = S.CheckVarTemplateId(VarTemplate, fakeLoc, fakeLoc, TLI);

}

TEST(ScopeReflectionTest, InstantiateVarTemplate) {
std::vector<Decl*> Decls;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'Decls' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::vector<Decl*> Decls;
std::vector<Decl*> Decls = 0;

GetAllTopLevelDecls(code, Decls);
ASTContext& C = Interp->getCI()->getASTContext();

std::vector<Cpp::TemplateArgInfo> args1 = {C.IntTy.getAsOpaquePtr()};
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'args1' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::vector<Cpp::TemplateArgInfo> args1 = {C.IntTy.getAsOpaquePtr()};
std::vector<Cpp::TemplateArgInfo> args1 = 0 = {C.IntTy.getAsOpaquePtr()};

}

TEST(ScopeReflectionTest, InstantiateFunctionTemplate) {
std::vector<Decl*> Decls;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'Decls' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::vector<Decl*> Decls;
std::vector<Decl*> Decls = 0;

GetAllTopLevelDecls(code, Decls);
ASTContext& C = Interp->getCI()->getASTContext();

std::vector<Cpp::TemplateArgInfo> args1 = {C.IntTy.getAsOpaquePtr()};
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'args1' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::vector<Cpp::TemplateArgInfo> args1 = {C.IntTy.getAsOpaquePtr()};
std::vector<Cpp::TemplateArgInfo> args1 = 0 = {C.IntTy.getAsOpaquePtr()};

@aaronj0
Copy link
Collaborator

aaronj0 commented Apr 4, 2024

@vgvassilev looks like all the bots are failing only on the cppyy and xeus builds due to the pending update in cppyy-backend compiler-research/cppyy-backend#94

I believe this PR can be merged now

@vgvassilev vgvassilev merged commit d5bef3c into compiler-research:main Apr 4, 2024
18 of 42 checks passed
@vgvassilev vgvassilev deleted the consolidate-template-instantiation branch April 4, 2024 16:22
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.

2 participants