Skip to content

Implement Demangle and IsClassPolymorphic APIs #451

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
Jan 18, 2025

Conversation

Vipul-Cariappa
Copy link
Collaborator

Description

Implements DemangleSymbol and IsClassPolymorphic APIs. These are needed to implement Cppyy::GetActualClass.

Fixes

Fixes 8 tests on cppyy, once Cppyy::GetActualClass is implemented.

  • Bug fix
  • New feature
  • Requires documentation updates

Testing

Implemented necessary tests.

Checklist

  • I have read the contribution guide recently

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

std::string DemangleSymbol(const std::string& mangled_name) {
char* demangle = itaniumDemangle(mangled_name);
std::string res(demangle);
std::free(demangle);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: calling legacy resource function without passing a 'gsl::owner<>' [cppcoreguidelines-owning-memory]

    std::free(demangle);
    ^

Copy link
Contributor

Choose a reason for hiding this comment

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

We should just assign to std::string

std::string DemangleSymbol(const std::string& mangled_name) {
char* demangle = itaniumDemangle(mangled_name);
std::string res(demangle);
std::free(demangle);
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 manage memory manually; use RAII [cppcoreguidelines-no-malloc]

    std::free(demangle);
    ^

std::string DemangleSymbol(const std::string& mangled_name) {
char* demangle = itaniumDemangle(mangled_name);
std::string res(demangle);
std::free(demangle);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::free" is directly included [misc-include-cleaner]

lib/Interpreter/CppInterOp.cpp:26:

- #if CLANG_VERSION_MAJOR >= 19
+ #include <cstdlib>
+ #if CLANG_VERSION_MAJOR >= 19

@@ -17,6 +17,48 @@ using namespace TestUtils;
using namespace llvm;
using namespace clang;

TEST(ScopeReflectionTest, DemangleSymbol) {
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 'TEST' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

TEST(ScopeReflectionTest, DemangleSymbol) {
^

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.04%. Comparing base (04f45b2) to head (a021250).
Report is 5 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #451      +/-   ##
==========================================
+ Coverage   70.96%   71.04%   +0.07%     
==========================================
  Files           9        9              
  Lines        3541     3550       +9     
==========================================
+ Hits         2513     2522       +9     
  Misses       1028     1028              
Files with missing lines Coverage Δ
include/clang/Interpreter/CppInterOp.h 96.29% <ø> (ø)
lib/Interpreter/CppInterOp.cpp 80.43% <100.00%> (+0.09%) ⬆️
Files with missing lines Coverage Δ
include/clang/Interpreter/CppInterOp.h 96.29% <ø> (ø)
lib/Interpreter/CppInterOp.cpp 80.43% <100.00%> (+0.09%) ⬆️

@Vipul-Cariappa Vipul-Cariappa force-pushed the dev/demangle branch 5 times, most recently from 8bb8cbf to e6ff45b Compare January 15, 2025 05:25
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

#include "clang/AST/ASTDumper.h"
#include "clang/AST/Decl.h"
#include "clang/AST/DeclBase.h"
#include "clang/AST/GlobalDecl.h"

#include "gtest/gtest.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'gtest/gtest.h' file not found [clang-diagnostic-error]

#include "gtest/gtest.h"
         ^


#include "gtest/gtest.h"

using namespace TestUtils;
using namespace llvm;
using namespace clang;

TEST(ScopeReflectionTest, DemangleSymbol) {
if (llvm::sys::RunningOnValgrind())
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "llvm::sys::RunningOnValgrind" is directly included [misc-include-cleaner]

unittests/CppInterOp/ScopeReflectionTest.cpp:14:

+ #include <llvm/Support/Valgrind.h>

if (llvm::sys::RunningOnValgrind())
GTEST_SKIP() << "XFAIL due to Valgrind report";

std::string code = R"(
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::string" is directly included [misc-include-cleaner]

unittests/CppInterOp/ScopeReflectionTest.cpp:14:

+ #include <string>


std::string mangled_add_int;
std::string mangled_add_double;
compat::maybeMangleDeclName(Add_int, mangled_add_int);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "compat::maybeMangleDeclName" is directly included [misc-include-cleaner]

unittests/CppInterOp/ScopeReflectionTest.cpp:3:

- #include "clang-c/CXCppInterOp.h"
+ #include "/github/workspace/lib/Interpreter/Compatibility.h"
+ #include "clang-c/CXCppInterOp.h"

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. Please update the pr and the commit messages accordingly before merging.

@Vipul-Cariappa Vipul-Cariappa changed the title Implement DemangleSymbol and IsClassPolymorphic APIs Implement Demangle and IsClassPolymorphic APIs Jan 17, 2025
@Vipul-Cariappa Vipul-Cariappa merged commit 8e064dc into compiler-research:main Jan 18, 2025
59 checks passed
@Vipul-Cariappa Vipul-Cariappa deleted the dev/demangle branch January 18, 2025 12:01
# 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