Skip to content

Update GetFunctionReturnType API for template functions #218

New issue

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

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

Already on GitHub? # to your account

Merged
merged 2 commits into from
Apr 7, 2024

Conversation

aaronj0
Copy link
Collaborator

@aaronj0 aaronj0 commented Apr 5, 2024

No description provided.

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

@@ -228,7 +228,7 @@ TEST(FunctionReflectionTest, GetFunctionsUsingName) {
}

TEST(FunctionReflectionTest, GetFunctionReturnType) {
std::vector<Decl*> Decls, SubDecls;
std::vector<Decl*> Decls, SubDecls, TemplateSubDecls;
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, SubDecls, TemplateSubDecls;
std::vector<Decl*> Decls = 0, SubDecls, TemplateSubDecls;

EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[5])), "Switch");
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[4])),
"double");
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[5])),
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]

  EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[5])),
                                                                  ^

EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[9])), "volatile N::C");
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[10])), "const volatile N::C");
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[11])), "NULL TYPE");
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[7])),
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 7 is a magic number; consider replacing it with a named constant [readability-magic-numbers]

  EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[7])),
                                                                  ^

EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[11])), "NULL TYPE");
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[7])),
"N::C *");
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[8])),
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 8 is a magic number; consider replacing it with a named constant [readability-magic-numbers]

  EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[8])),
                                                                  ^

"N::C *");
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[8])),
"const N::C");
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[9])),
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 9 is a magic number; consider replacing it with a named constant [readability-magic-numbers]

  EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[9])),
                                                                  ^

"const N::C");
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[9])),
"volatile N::C");
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[10])),
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 10 is a magic number; consider replacing it with a named constant [readability-magic-numbers]

  EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[10])),
                                                                  ^

"volatile N::C");
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[10])),
"const volatile N::C");
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[11])),
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 11 is a magic number; consider replacing it with a named constant [readability-magic-numbers]

  EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetFunctionReturnType(Decls[11])),
                                                                  ^

@@ -822,6 +822,10 @@ namespace Cpp {
return FD->getReturnType().getAsOpaquePtr();
}

if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionTemplateDecl>(D)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can D be null here? If not we need to drop the _or_null part.

Copy link

codecov bot commented Apr 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.82%. Comparing base (32efeba) to head (173fea8).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #218      +/-   ##
==========================================
+ Coverage   78.80%   78.82%   +0.01%     
==========================================
  Files           8        8              
  Lines        3100     3102       +2     
==========================================
+ Hits         2443     2445       +2     
  Misses        657      657              
Files Coverage Δ
lib/Interpreter/CppInterOp.cpp 86.44% <100.00%> (+0.01%) ⬆️
Files Coverage Δ
lib/Interpreter/CppInterOp.cpp 86.44% <100.00%> (+0.01%) ⬆️

@aaronj0 aaronj0 force-pushed the update-returntype-api branch from ca6e56c to 173fea8 Compare April 5, 2024 15:11
Copy link
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!

@vgvassilev vgvassilev merged commit c13391b into compiler-research:main Apr 7, 2024
44 checks passed
@aaronj0 aaronj0 deleted the update-returntype-api branch April 22, 2024 12:44
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants