Add `GetBinaryOperator` #319

Vipul-Cariappa · 2024-08-31T14:25:08Z

Implements GetBinaryOperator to resolve binary operator in the given scope with specified arguments.

Vipul-Cariappa · 2024-08-31T14:26:54Z

lib/Interpreter/CppInterOp.cpp

+    else if (op == "^")
+      getSema().LookupOverloadedOperatorName(clang::OO_Caret, S, lookup);


What other operators should be here?
Should I include +=, &=, &&, etc?

We probably should create some enum with the supported operators.

vgvassilev

We need a test case for this PR.

vgvassilev · 2024-08-31T14:30:39Z

lib/Interpreter/CppInterOp.cpp

+    else if (op == "^")
+      getSema().LookupOverloadedOperatorName(clang::OO_Caret, S, lookup);


We probably should create some enum with the supported operators.

github-actions

clang-tidy made some suggestions

github-actions · 2024-08-31T14:32:27Z

lib/Interpreter/CppInterOp.cpp

+      getSema().LookupOverloadedOperatorName(clang::OO_Caret, S, lookup);
+
+    for (NamedDecl* x : lookup) {
+      if (auto F = llvm::dyn_cast<Decl>(x)) {


warning: 'auto F' can be declared as 'auto *F' [llvm-qualified-auto]

Suggested change

if (auto F = llvm::dyn_cast<Decl>(x)) {

if (auto *F = llvm::dyn_cast<Decl>(x)) {

vgvassilev · 2024-08-31T14:33:26Z

lib/Interpreter/CppInterOp.cpp

+    clang::UnresolvedSet<8> lookup;
+
+    if (op == "+")
+      getSema().LookupOverloadedOperatorName(clang::OO_Plus, S, lookup);


Looks like Sema::LookupBinOp is a higher-level interface. We should probably use it instead...

codecov · 2024-08-31T14:33:57Z

Codecov Report

Attention: Patch coverage is 84.61538% with 4 lines in your changes missing coverage. Please review.

Project coverage is 73.47%. Comparing base (91e392d) to head (ead101e).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/Interpreter/CppInterOp.cpp	84.61%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #319      +/-   ##
==========================================
+ Coverage   73.38%   73.47%   +0.09%     
==========================================
  Files           8        8              
  Lines        2979     3005      +26     
==========================================
+ Hits         2186     2208      +22     
- Misses        793      797       +4

Files with missing lines	Coverage Δ
include/clang/Interpreter/CppInterOp.h	`96.00% <ø> (ø)`
lib/Interpreter/CppInterOp.cpp	`79.20% <84.61%> (+0.08%)`	⬆️

Files with missing lines	Coverage Δ
include/clang/Interpreter/CppInterOp.h	`96.00% <ø> (ø)`
lib/Interpreter/CppInterOp.cpp	`79.20% <84.61%> (+0.08%)`	⬆️

github-actions · 2024-09-03T06:05:47Z

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

vgvassilev · 2024-09-03T06:15:07Z

include/clang/Interpreter/CppInterOp.h

@@ -465,7 +483,7 @@ namespace Cpp {
                                                TCppIndex_t param_index);

  ///\returns function that performs operation op on lc and rc
-  TCppFunction_t GetBinaryOperator(TCppScope_t scope, const std::string& op,
+  TCppFunction_t GetBinaryOperator(TCppScope_t scope, enum BinaryOperator op,


We don't need an elaborated type here. Dropping the enum should be fine.

I was thinking of removing our definition of the enum and replacing it with using clang::BinaryOperatorKind;. Then we will not require that long switch ... case ... in CppInterOp.cpp::GetBinaryOperator. Nor any kind of string comparison.
Any thoughts?

We can't leak implementation details for clang. That means we can't include header files of llvm/clang in CppInterOp.h.

vgvassilev · 2024-09-03T06:16:05Z

lib/Interpreter/CppInterOp.cpp

@@ -3178,35 +3178,53 @@ namespace Cpp {

    clang::UnresolvedSet<8> lookup;

-    if (op == "+")
+    switch (op) {
+    case Plus:
      getSema().LookupOverloadedOperatorName(clang::OO_Plus, S, lookup);


Can't we follow the enum ordering in clang. Eg clang::OO_Plus == BinaryOperator.Plus? Then this will be just a single call instead of a switch stmt.

unittests/CppInterOp/ScopeReflectionTest.cpp

github-actions · 2024-09-03T06:26:56Z

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

github-actions · 2024-09-03T07:09:12Z

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

Vipul-Cariappa · 2024-09-03T07:19:38Z

Hi @vgvassilev, I have made all the changes you suggested. Please have a look.

Valgrind test fails with Conditional jump or move depends on uninitialised value. I am not sure about the cause here.

unittests/CppInterOp/ScopeReflectionTest.cpp

github-actions · 2024-09-03T08:24:57Z

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

github-actions · 2024-09-05T07:12:22Z

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

vgvassilev

LGTM! See remaining comments.

vgvassilev · 2024-09-05T10:15:37Z

lib/Interpreter/CppInterOp.cpp

+    for (NamedDecl* x : lookup) {
+      if (auto* F = llvm::dyn_cast<Decl>(x)) {


Suggested change

for (NamedDecl* x : lookup) {

if (auto* F = llvm::dyn_cast<Decl>(x)) {

for (NamedDecl* D : lookup) {

if (auto* FD = llvm::dyn_cast<Decl>(D)) {

This is the common convention.

unittests/CppInterOp/ScopeReflectionTest.cpp

Vipul-Cariappa · 2024-09-05T13:21:07Z

I have spent more than 3 hours trying to get the following to work.

// C++ code

class MyClass {
public:
    int x;
    MyClass(int x) : x(x) {}
};

namespace MyScope {

    MyClass operator*(MyClass lhs, MyClass rhs) {
        return MyClass(lhs.x * rhs.x);
    }

} // end namespace MyScope

Look up the operator with

Cpp::GetBinaryOperator(Cpp::GetScope("MyScope"), Cpp::BinaryOperator::Mul, "MyClass", "MyClass");

But it is not working.

This is required for the operator+ for std::string; it is defined within the std namespace.

github-actions · 2024-09-05T13:22:34Z

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

Add GetBinaryOperator

63ee219

Vipul-Cariappa commented Aug 31, 2024

View reviewed changes

vgvassilev reviewed Aug 31, 2024

View reviewed changes

github-actions bot reviewed Aug 31, 2024

View reviewed changes

vgvassilev reviewed Aug 31, 2024

View reviewed changes

Vipul-Cariappa added 2 commits September 3, 2024 05:55

Use enum for operator

f9b5d9c

Add test case for GetBinaryOperator

076559a

vgvassilev reviewed Sep 3, 2024

View reviewed changes

Use LookupBinOp instead of LookupOverloadedOperatorName

8851fff

enum BinaryOperator is an alias of clang::BinaryOperatorKind

961fbf3

aaronj0 reviewed Sep 3, 2024

View reviewed changes

unittests/CppInterOp/ScopeReflectionTest.cpp Show resolved Hide resolved

Skip test on valgrind(codegen uninitialised val)

e8254fa

Test different cases

e4076df

vgvassilev approved these changes Sep 5, 2024

View reviewed changes

Update test to lookup in global scope instead of std

ead101e

vgvassilev merged commit 9bd4678 into compiler-research:main Sep 5, 2024
59 checks passed

Vipul-Cariappa deleted the GetBinaryOperator branch September 6, 2024 09:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `GetBinaryOperator` #319

Add `GetBinaryOperator` #319

Vipul-Cariappa commented Aug 31, 2024

Vipul-Cariappa Aug 31, 2024

vgvassilev Aug 31, 2024

vgvassilev left a comment

vgvassilev Aug 31, 2024

github-actions bot left a comment

github-actions bot Aug 31, 2024

vgvassilev Aug 31, 2024

codecov bot commented Aug 31, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024

vgvassilev Sep 3, 2024

Vipul-Cariappa Sep 3, 2024

vgvassilev Sep 3, 2024

vgvassilev Sep 3, 2024

github-actions bot commented Sep 3, 2024

github-actions bot commented Sep 3, 2024

Vipul-Cariappa commented Sep 3, 2024

github-actions bot commented Sep 3, 2024

github-actions bot commented Sep 5, 2024

vgvassilev left a comment

vgvassilev Sep 5, 2024

Vipul-Cariappa commented Sep 5, 2024

github-actions bot commented Sep 5, 2024

		else if (op == "^")
		getSema().LookupOverloadedOperatorName(clang::OO_Caret, S, lookup);

	if (auto F = llvm::dyn_cast<Decl>(x)) {
	if (auto *F = llvm::dyn_cast<Decl>(x)) {

		for (NamedDecl* x : lookup) {
		if (auto* F = llvm::dyn_cast<Decl>(x)) {

Add GetBinaryOperator #319

Add GetBinaryOperator #319

Conversation

Vipul-Cariappa commented Aug 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgvassilev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Aug 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 31, 2024 • edited Loading

Codecov Report

github-actions bot commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 3, 2024

github-actions bot commented Sep 3, 2024

Vipul-Cariappa commented Sep 3, 2024

github-actions bot commented Sep 3, 2024

github-actions bot commented Sep 5, 2024

vgvassilev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vipul-Cariappa commented Sep 5, 2024

github-actions bot commented Sep 5, 2024

Add `GetBinaryOperator` #319

Add `GetBinaryOperator` #319

codecov bot commented Aug 31, 2024 •

edited

Loading