Update `GetBinaryOperator` to `GetOperator` #394

Vipul-Cariappa · 2024-12-16T05:13:34Z

Description

Resolving unary operators and operators defined within namespaces.
Required for + op in std::string.

Fixes (issue)

Three issues at cppyy. Requires a patch for both CPyCppyy and cppyy-backend.

Type of change

Please tick all options which are relevant.

Bug fix
New feature
Requires documentation updates

Testing

I have added in tests cases for unary operator lookup and lookup of operator within a namespace.

Checklist

I have read the contribution guide recently

github-actions

clang-tidy made some suggestions

include/clang/Interpreter/CppInterOp.h

lib/Interpreter/CppInterOp.cpp

codecov · 2024-12-16T05:23:07Z

Codecov Report

Attention: Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 70.85%. Comparing base (8c0cd6d) to head (6cf8923).
Report is 3 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #394      +/-   ##
==========================================
- Coverage   70.87%   70.85%   -0.02%     
==========================================
  Files           9        9              
  Lines        3533     3538       +5     
==========================================
+ Hits         2504     2507       +3     
- Misses       1029     1031       +2

Files with missing lines	Coverage Δ
include/clang/Interpreter/CppInterOp.h	`96.29% <ø> (ø)`
lib/Interpreter/CppInterOp.cpp	`80.14% <81.25%> (-0.06%)`	⬇️

Files with missing lines	Coverage Δ
include/clang/Interpreter/CppInterOp.h	`96.29% <ø> (ø)`
lib/Interpreter/CppInterOp.cpp	`80.14% <81.25%> (-0.06%)`	⬇️

vgvassilev

Maybe address the clang-tidy issue.

include/clang/Interpreter/CppInterOp.h

lib/Interpreter/CppInterOp.cpp

github-actions

clang-tidy made some suggestions

github-actions · 2024-12-18T07:13:47Z

include/clang/Interpreter/CppInterOp.h

+    OP_Coawait,
+  };
+
+  enum OperatorKind {


warning: enum 'OperatorKind' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]

enum OperatorKind { ^

lib/Interpreter/CppInterOp.cpp

github-actions · 2024-12-18T07:13:47Z

lib/Interpreter/CppInterOp.cpp

+    if (auto* FD = llvm::dyn_cast<FunctionDecl>(D)) {
+      if (FD->isOverloadedOperator()) {
+        switch (FD->getOverloadedOperator()) {
+#define OVERLOADED_OPERATOR(Name, Spelling, Token, Unary, Binary,            \


warning: function-like macro 'OVERLOADED_OPERATOR' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]

#define OVERLOADED_OPERATOR(Name, Spelling, Token, Unary, Binary, \ ^

That type of check should be disabled in our clang-tidy config..

lib/Interpreter/CppInterOp.cpp

github-actions

clang-tidy made some suggestions

lib/Interpreter/CppInterOp.cpp

vgvassilev · 2024-12-18T07:58:14Z

include/clang/Interpreter/CppInterOp.h

+    kUnary = 0b001,
+    kBinary = 0b010,
+    kBoth = 0b011,


I guess we won't or these, 1,2,3 should be fine...

vgvassilev · 2024-12-18T07:59:43Z

include/clang/Interpreter/CppInterOp.h

+    OP_Coawait,
+  };
+
+  enum OperatorKind {


Suggested change

enum OperatorKind {

enum OperatorArity {

github-actions

clang-tidy made some suggestions

github-actions · 2024-12-18T09:40:53Z

include/clang/Interpreter/CppInterOp.h

+    OP_Coawait,
  };

+  enum OperatorArity { kUnary = 1, kBinary, kBoth };


warning: enum 'OperatorArity' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]

enum OperatorArity { kUnary = 1, kBinary, kBoth }; ^

github-actions · 2024-12-18T09:40:53Z

lib/Interpreter/CppInterOp.cpp


-  void GetBinaryOperator(TCppScope_t scope, enum BinaryOperator op,
-                         std::vector<TCppFunction_t>& operators) {
+  bool IsUnaryOperator(TCppScope_t scope) {


warning: redundant explicit casting to the same type 'Expr *' as the sub-expression, remove this casting [readability-redundant-casting]

Suggested change

bool IsUnaryOperator(TCppScope_t scope) {

Expr *DefaultArgExpr = PI->getDefaultArg();

Additional context

llvm/include/clang/AST/Decl.h:1809: source type originates from the invocation of this method

Expr *getDefaultArg(); ^

github-actions · 2024-12-18T09:40:53Z

lib/Interpreter/CppInterOp.cpp

+        switch (FD->getOverloadedOperator()) {
+#define OVERLOADED_OPERATOR(Name, Spelling, Token, Unary, Binary,            \
+                              MemberOnly)                                      \
+    case OO_##Name:                                                            \


warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

auto *D = (clang::Decl *)method; ^

github-actions · 2024-12-18T09:40:53Z

lib/Interpreter/CppInterOp.cpp

+      }
+    }
+    return false;
+  }


warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

auto *D = (clang::Decl *)func; ^

github-actions · 2024-12-18T09:40:53Z

lib/Interpreter/CppInterOp.cpp

+    if (auto* DC = llvm::dyn_cast_or_null<DeclContext>(D)) {
+      ASTContext& C = getSema().getASTContext();
+      DeclContextLookupResult Result =
+          DC->lookup(C.DeclarationNames.getCXXOperatorName(


warning: Called C++ object pointer is null [clang-analyzer-core.CallAndMessage]

return PI->getNameAsString(); ^

Additional context

lib/Interpreter/CppInterOp.cpp:3299: 'PI' initialized to a null pointer value

clang::ParmVarDecl* PI = nullptr; ^

lib/Interpreter/CppInterOp.cpp:3301: Assuming null pointer is passed into cast

if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionDecl>(D)) ^

lib/Interpreter/CppInterOp.cpp:3301: 'FD' is null

if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionDecl>(D)) ^

lib/Interpreter/CppInterOp.cpp:3301: Taking false branch

if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionDecl>(D)) ^

lib/Interpreter/CppInterOp.cpp:3303: Assuming null pointer is passed into cast

else if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionTemplateDecl>(D)) ^

lib/Interpreter/CppInterOp.cpp:3303: 'FD' is null

else if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionTemplateDecl>(D)) ^

lib/Interpreter/CppInterOp.cpp:3303: Taking false branch

else if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionTemplateDecl>(D)) ^

lib/Interpreter/CppInterOp.cpp:3306: Called C++ object pointer is null

return PI->getNameAsString(); ^

vgvassilev · 2024-12-18T15:43:13Z

include/clang/Interpreter/CppInterOp.h

+    OP_Coawait,
  };

+  enum OperatorArity { kUnary = 1, kBinary, kBoth };


Suggested change

enum OperatorArity { kUnary = 1, kBinary, kBoth };

enum OperatorArity { kBoth = 0, kUnary, kBinary };

I did not understand this.
All Unary operators are not Binary operators. So kBoth = 0 and kUnary = 0 is confusing. Example ~ operator.

Sorry, my bad, fixed the suggestion.

vgvassilev · 2024-12-18T15:43:33Z

include/clang/Interpreter/CppInterOp.h

-  void GetBinaryOperator(TCppScope_t scope, enum BinaryOperator op,
-                         std::vector<TCppFunction_t>& operators);
+  ///\returns true if scope is a unary operator
+  bool IsUnaryOperator(TCppScope_t scope);


Suggested change

bool IsUnaryOperator(TCppScope_t scope);

bool IsUnaryOperator(TCppFunction_t op);

vgvassilev · 2024-12-18T15:43:48Z

include/clang/Interpreter/CppInterOp.h

+  bool IsUnaryOperator(TCppScope_t scope);
+
+  ///\returns true if scope is a binary operator
+  bool IsBinaryOperator(TCppScope_t scope);


Suggested change

bool IsBinaryOperator(TCppScope_t scope);

bool IsBinaryOperator(TCppFunction_t op);

vgvassilev · 2024-12-18T15:44:33Z

lib/Interpreter/CppInterOp.cpp

+    if (auto* FD = llvm::dyn_cast<FunctionDecl>(D)) {
+      if (FD->isOverloadedOperator()) {
+        switch (FD->getOverloadedOperator()) {
+#define OVERLOADED_OPERATOR(Name, Spelling, Token, Unary, Binary,            \


That type of check should be disabled in our clang-tidy config..

vgvassilev · 2024-12-18T15:51:55Z

lib/Interpreter/CppInterOp.cpp

+    if (auto* FD = llvm::dyn_cast<FunctionDecl>(D)) {
+      if (FD->isOverloadedOperator()) {
+        switch (FD->getOverloadedOperator()) {
+#define OVERLOADED_OPERATOR(Name, Spelling, Token, Unary, Binary,            \
+                              MemberOnly)                                      \
+    case OO_##Name:                                                            \
+      return Unary;
+#include "clang/Basic/OperatorKinds.def"
+        default:
+          break;
+        }
+      }
+    }


We should move that into a utility function, write the switch such that it returns

OperatorArity getOperatorArity(...) { if (Unary && Binary) return kBoth; ... }

github-actions

clang-tidy made some suggestions

github-actions · 2024-12-19T11:49:50Z

include/clang/Interpreter/CppInterOp.h

+    OP_Coawait,
  };

+  enum OperatorArity { kNone, kUnary, kBinary, kBoth };


warning: enum 'OperatorArity' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]

enum OperatorArity { kNone, kUnary, kBinary, kBoth }; ^

vgvassilev · 2024-12-19T12:02:31Z

lib/Interpreter/CppInterOp.cpp

+        }
+      }
+    }
+    return kNone;


That should be unreachable, right? What does it mean arity::kNone? An operator without arguments? I do not think that's possible.

Yes, should be unreachable.
It is for cases when the argument is something other than an operator or a function that is not an operator.

Shouldn't that be an assert or an early exit?

I will say this is simpler. Assert will crash the program. In this case, the user can handle kNone as an error.

Also if I remove the return, clang-tidy will get angry.

Let me know if you still think we should remove kNone. I can do that, but not sure how I would satisfy clang-tidy's message of Non-void function does not return a value in all control paths.

You should remove it. You can return ~0U

Compiler error:

/home/vipul-cariappa/Workspace/cpp-py/compiler-research/CppInterOp/lib/Interpreter/CppInterOp.cpp:3339:12: error: invalid conversion from ‘unsigned int’ to ‘Cpp::OperatorArity’ [-fpermissive] 3339 | return ~0U; | ^~~ | | | unsigned int

Should I rename it to kInvalid instead?

Cast it to OperatorArity?

vgvassilev · 2024-12-19T12:03:13Z

lib/Interpreter/CppInterOp.cpp

-      if (auto* FD = llvm::dyn_cast<Decl>(D))
-        operators.push_back(FD);
+      for (auto* i : Result) {
+        if (kind & getOperatorArity(i))


Suggested change

if (kind & getOperatorArity(i))

if (kind == getOperatorArity(i))

When kind = kBoth should we return Unary $\cap$ Binary or Unary $\cup$ Binary?
This logic using & returns Unary $\cup$ Binary.
Whereas == will return Unary $\cap$ Binary.

Operators like - is both Unary and Binary.

I set the enum OperatorArity values so that I can perform this bitwise operation.

When I say kBoth, I expect all unary and binary operators to be present in that list.

In that case, we should be using &. kUnary = 0b001 and kBinary = 0b010 and kBoth = kUnary ^ kBinary = 0b011.
No changes are required here.

vgvassilev · 2024-12-20T06:23:26Z

Do we know why the check if failing?

aaronj0 · 2024-12-20T06:56:14Z

Do we know why the check if failing?

Looks like the API update in cppyy-backend was not pulled, rerunning now. should be green

github-actions

clang-tidy made some suggestions

github-actions · 2024-12-20T07:22:44Z

lib/Interpreter/CppInterOp.cpp

+        }
+      }
+    }
+    return (OperatorArity)~0U;


warning: The value '4294967295' provided to the cast expression is not in the valid range of values for 'OperatorArity' [clang-analyzer-optin.core.EnumCastOutOfRange]

return (OperatorArity)~0U; ^

Additional context

include/clang/Interpreter/CppInterOp.h:88: enum declared here

enum OperatorArity { kUnary = 1, kBinary, kBoth }; ^

lib/Interpreter/CppInterOp.cpp:3319: Assuming 'D' is not a 'CastReturnType'

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

lib/Interpreter/CppInterOp.cpp:3319: 'FD' is null

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

lib/Interpreter/CppInterOp.cpp:3319: Taking false branch

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

lib/Interpreter/CppInterOp.cpp:3338: The value '4294967295' provided to the cast expression is not in the valid range of values for 'OperatorArity'

return (OperatorArity)~0U; ^

resolving unary operators and operators defined within namespaces

vgvassilev

LGTM!

`test03_stllike_preinc` fixed by compiler-research/CppInterOp#401 others fixed by compiler-research/CppInterOp#394

Vipul-Cariappa requested review from aaronj0 and vgvassilev December 16, 2024 05:13

github-actions bot reviewed Dec 16, 2024

View reviewed changes

include/clang/Interpreter/CppInterOp.h Show resolved Hide resolved

lib/Interpreter/CppInterOp.cpp Outdated Show resolved Hide resolved

Vipul-Cariappa force-pushed the dev/operators branch from 3c7449a to 5f7c378 Compare December 16, 2024 05:42

vgvassilev reviewed Dec 17, 2024

View reviewed changes

include/clang/Interpreter/CppInterOp.h Outdated Show resolved Hide resolved

lib/Interpreter/CppInterOp.cpp Outdated Show resolved Hide resolved

vgvassilev reviewed Dec 17, 2024

View reviewed changes

lib/Interpreter/CppInterOp.cpp Show resolved Hide resolved

Vipul-Cariappa force-pushed the dev/operators branch 2 times, most recently from 371ef22 to 709371e Compare December 18, 2024 07:13

github-actions bot reviewed Dec 18, 2024

View reviewed changes

lib/Interpreter/CppInterOp.cpp Show resolved Hide resolved

vgvassilev reviewed Dec 18, 2024

View reviewed changes

Vipul-Cariappa force-pushed the dev/operators branch from 709371e to 8277fe9 Compare December 18, 2024 09:31

github-actions bot reviewed Dec 18, 2024

View reviewed changes

Vipul-Cariappa force-pushed the dev/operators branch from 8277fe9 to fb19bfc Compare December 18, 2024 15:24

vgvassilev reviewed Dec 18, 2024

View reviewed changes

Vipul-Cariappa force-pushed the dev/operators branch from fb19bfc to 2c83c8d Compare December 19, 2024 11:43

github-actions bot reviewed Dec 19, 2024

View reviewed changes

vgvassilev reviewed Dec 19, 2024

View reviewed changes

Vipul-Cariappa force-pushed the dev/operators branch from 2c83c8d to ce7d778 Compare December 19, 2024 14:10

Vipul-Cariappa force-pushed the dev/operators branch from ce7d778 to b32d28b Compare December 20, 2024 07:16

github-actions bot reviewed Dec 20, 2024

View reviewed changes

Vipul-Cariappa added 2 commits January 2, 2025 11:52

Update GetBinaryOperator to GetOperator

9cec120

resolving unary operators and operators defined within namespaces

disable macro checks in clang-tidy

6cf8923

Vipul-Cariappa force-pushed the dev/operators branch from b32d28b to 6cf8923 Compare January 2, 2025 06:30

vgvassilev approved these changes Jan 2, 2025

View reviewed changes

Vipul-Cariappa merged commit c446c3e into compiler-research:main Jan 4, 2025
47 checks passed

Vipul-Cariappa deleted the dev/operators branch January 4, 2025 03:07

Vipul-Cariappa mentioned this pull request Jan 4, 2025

Implement GetGlobalOperator compiler-research/cppyy-backend#119

Merged

Vipul-Cariappa added a commit to Vipul-Cariappa/cppyy-compiler-research that referenced this pull request Jan 6, 2025

[test] update tags

5cb3c36

`test03_stllike_preinc` fixed by compiler-research/CppInterOp#401 others fixed by compiler-research/CppInterOp#394

Vipul-Cariappa mentioned this pull request Jan 6, 2025

[test] update tags compiler-research/cppyy#117

Merged

Vipul-Cariappa added a commit to compiler-research/cppyy that referenced this pull request Jan 6, 2025

[test] update tags (#117)

8392327

`test03_stllike_preinc` fixed by compiler-research/CppInterOp#401 others fixed by compiler-research/CppInterOp#394

	bool IsUnaryOperator(TCppScope_t scope) {
	Expr *DefaultArgExpr = PI->getDefaultArg();

	enum OperatorArity { kUnary = 1, kBinary, kBoth };
	enum OperatorArity { kBoth = 0, kUnary, kBinary };

	bool IsUnaryOperator(TCppScope_t scope);
	bool IsUnaryOperator(TCppFunction_t op);

	bool IsBinaryOperator(TCppScope_t scope);
	bool IsBinaryOperator(TCppFunction_t op);

	if (kind & getOperatorArity(i))
	if (kind == getOperatorArity(i))

Update GetBinaryOperator to GetOperator #394

Update GetBinaryOperator to GetOperator #394

Uh oh!

Conversation

Vipul-Cariappa commented Dec 16, 2024

Description

Fixes (issue)

Type of change

Testing

Checklist

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vgvassilev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

vgvassilev Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Update `GetBinaryOperator` to `GetOperator` #394

Update `GetBinaryOperator` to `GetOperator` #394

codecov bot commented Dec 16, 2024 •

edited

Loading

vgvassilev Dec 18, 2024 •

edited

Loading