resolve static constexpr class attribute #351

Vipul-Cariappa · 2024-11-12T15:02:48Z

No description provided.

codecov · 2024-11-12T15:11:51Z

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.57%. Comparing base (826be78) to head (1d4a678).
Report is 1 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #351      +/-   ##
==========================================
+ Coverage   74.45%   74.57%   +0.12%     
==========================================
  Files           8        8              
  Lines        3206     3214       +8     
==========================================
+ Hits         2387     2397      +10     
+ Misses        819      817       -2

Files with missing lines	Coverage Δ
lib/Interpreter/CppInterOp.cpp	`80.10% <88.88%> (+0.08%)`	⬆️

... and 1 file with indirect coverage changes

Files with missing lines	Coverage Δ
lib/Interpreter/CppInterOp.cpp	`80.10% <88.88%> (+0.08%)`	⬆️

... and 1 file with indirect coverage changes

github-actions

clang-tidy made some suggestions

github-actions · 2024-11-12T15:11:58Z

lib/Interpreter/CppInterOp.cpp

+            (VD->isConstexpr() || VD->getType().isConstQualified())) {
+          if (const APValue* val = VD->evaluateValue()) {
+            if (VD->getType()->isIntegralType(C)) {
+              return (intptr_t)val->getInt().getRawData();


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

return (intptr_t)val->getInt().getRawData(); ^

unittests/CppInterOp/FunctionReflectionTest.cpp

github-actions

clang-tidy made some suggestions

lib/Interpreter/CppInterOp.cpp

Vipul-Cariappa · 2024-11-13T04:37:51Z

Hi @vgvassilev, please look at the new changes. I am now using Sema::InstantiateVariableDefinition instead of Cpp::Declare to initialize the static constexpr.

I see some failing tests, that are due to the OSX dependency thing, rerunning them should fix it. I do not have the necessary permissions to rerun them.

vgvassilev · 2024-11-13T06:01:43Z

Hi @vgvassilev, please look at the new changes. I am now using Sema::InstantiateVariableDefinition instead of Cpp::Declare to initialize the static constexpr.

I see some failing tests, that are due to the OSX dependency thing, rerunning them should fix it. I do not have the necessary permissions to rerun them.

Can you accept the invite and retry?

vgvassilev · 2024-11-13T06:03:31Z

lib/Interpreter/CppInterOp.cpp

+      llvm::logAllUnhandledErrors(VDAorErr.takeError(), llvm::errs(),
+                                  "Failed to GetVariableOffset:");
+
+      if (VD->isConstexpr() && !VD->hasInit())


Shouldn’t here the check be if the VD is a template, implicitly instantiated with no init?

Looks like we don’t have test coverage for this case though…

Shouldn’t here the check be if the VD is a template, implicitly instantiated with no init?

!VD->hasInit() satisfies the "no init" part. Checking for template may not be required.

Looks like we don’t have test coverage for this case though…

Yes. The test case does not cover it. Running the cppyy test suit, it gets covered there. But I am not able to reproduce that here.

github-actions

clang-tidy made some suggestions

unittests/CppInterOp/FunctionReflectionTest.cpp

Vipul-Cariappa · 2024-11-13T14:36:14Z

Looks like cppyy-cling backend fails.

vgvassilev · 2024-11-13T14:10:17Z

lib/Interpreter/CppInterOp.cpp

-        llvm::logAllUnhandledErrors(VDAorErr.takeError(), llvm::errs(),
-                                    "Failed to GetVariableOffset:");
-        return 0;
+      if (VDAorErr)


This inversion seems wrong. Later, unconditionally we call logAllUnhandledErrors.

vgvassilev · 2024-11-13T14:10:47Z

lib/Interpreter/CppInterOp.cpp

+        getSema().InstantiateVariableDefinition(SourceLocation(), VD);
+      if (VD->hasInit() &&
+          (VD->isConstexpr() || VD->getType().isConstQualified())) {
+        if (const APValue* val = VD->evaluateValue()) {


These operations are cheaper and probably should go before the heavy getSymbolAddress?

I did the same with 24c82c5 commit.
This change happened to resolve things sooner and getSymbolAddress was never executed. Please look at the code coverage results of the same commit.

Sorry for the force pushes

github-actions

clang-tidy made some suggestions

lib/Interpreter/CppInterOp.cpp

github-actions · 2024-11-15T05:39:02Z

unittests/CppInterOp/VariableReflectionTest.cpp

+  EXPECT_EQ(datamembers.size(), 1);
+
+  intptr_t offset = Cpp::GetVariableOffset(datamembers[0]);
+  EXPECT_EQ(3, *(int*)offset);


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

EXPECT_EQ(3, *(int*)offset); ^

github-actions · 2024-11-15T05:39:02Z

unittests/CppInterOp/VariableReflectionTest.cpp

+  EXPECT_EQ(datamembers.size(), 1);
+
+  intptr_t offset = Cpp::GetVariableOffset(datamembers[0]);
+  EXPECT_EQ(3, *(int*)offset);


warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]

EXPECT_EQ(3, *(int*)offset); ^

github-actions · 2024-11-15T05:39:02Z

unittests/CppInterOp/VariableReflectionTest.cpp

+  EXPECT_EQ(datamembers.size(), 1);
+
+  offset = Cpp::GetVariableOffset(datamembers[0]);
+  EXPECT_EQ(5, *(int*)offset);


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

EXPECT_EQ(5, *(int*)offset); ^

github-actions · 2024-11-15T05:39:02Z

unittests/CppInterOp/VariableReflectionTest.cpp

+  EXPECT_EQ(datamembers.size(), 1);
+
+  offset = Cpp::GetVariableOffset(datamembers[0]);
+  EXPECT_EQ(5, *(int*)offset);


warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]

EXPECT_EQ(5, *(int*)offset); ^

github-actions · 2024-11-15T05:39:02Z

unittests/CppInterOp/VariableReflectionTest.cpp

+  EXPECT_EQ(datamembers.size(), 1);
+
+  offset = Cpp::GetVariableOffset(datamembers[0]);
+  EXPECT_EQ(2, *(int*)offset);


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

EXPECT_EQ(2, *(int*)offset); ^

github-actions · 2024-11-15T05:39:02Z

unittests/CppInterOp/VariableReflectionTest.cpp

+  EXPECT_EQ(datamembers.size(), 1);
+
+  offset = Cpp::GetVariableOffset(datamembers[0]);
+  EXPECT_EQ(2, *(int*)offset);


warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]

EXPECT_EQ(2, *(int*)offset); ^

by explicitly initializing constexpr with `Sema::InstantiateVariableDefinition`

fix for the osx dependency issue

lib/Interpreter/CppInterOp.cpp

vgvassilev

LGTM!

fixed by compiler-research/cppyy-backend#115 & compiler-research/CppInterOp#351

github-actions bot reviewed Nov 12, 2024

View reviewed changes

Vipul-Cariappa force-pushed the tuple-fix branch from 24c82c5 to e4cec60 Compare November 12, 2024 15:59

github-actions bot reviewed Nov 12, 2024

View reviewed changes

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

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

Vipul-Cariappa force-pushed the tuple-fix branch 2 times, most recently from 64b70fd to a99f1a0 Compare November 13, 2024 04:32

vgvassilev reviewed Nov 13, 2024

View reviewed changes

Vipul-Cariappa marked this pull request as draft November 13, 2024 06:10

Vipul-Cariappa force-pushed the tuple-fix branch from a99f1a0 to e034eab Compare November 13, 2024 14:07

github-actions bot reviewed Nov 13, 2024

View reviewed changes

unittests/CppInterOp/FunctionReflectionTest.cpp Outdated Show resolved Hide resolved

unittests/CppInterOp/FunctionReflectionTest.cpp Outdated Show resolved Hide resolved

Vipul-Cariappa marked this pull request as ready for review November 13, 2024 14:35

vgvassilev reviewed Nov 13, 2024

View reviewed changes

Vipul-Cariappa force-pushed the tuple-fix branch from e034eab to 97a9b6c Compare November 15, 2024 05:29

github-actions bot reviewed Nov 15, 2024

View reviewed changes

fix GetVariableOffset for static constexpr class attribute

94a59b8

by explicitly initializing constexpr with `Sema::InstantiateVariableDefinition`

Vipul-Cariappa force-pushed the tuple-fix branch from 97a9b6c to 94a59b8 Compare November 15, 2024 05:48

brew remove unxip on OSX-ARM exclusively

35346a7

fix for the osx dependency issue

Vipul-Cariappa force-pushed the tuple-fix branch from affdf00 to 35346a7 Compare November 15, 2024 13:45

vgvassilev reviewed Nov 15, 2024

View reviewed changes

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

Update lib/Interpreter/CppInterOp.cpp

1d4a678

Vipul-Cariappa force-pushed the tuple-fix branch from 95bde80 to 1d4a678 Compare November 16, 2024 08:06

vgvassilev approved these changes Nov 16, 2024

View reviewed changes

vgvassilev merged commit e4adda3 into compiler-research:main Nov 16, 2024
55 checks passed

Vipul-Cariappa deleted the tuple-fix branch November 17, 2024 03:46

Vipul-Cariappa added a commit to Vipul-Cariappa/cppyy-compiler-research that referenced this pull request Nov 28, 2024

[test] update tags

c32cc49

fixed by compiler-research/cppyy-backend#115 & compiler-research/CppInterOp#351

Vipul-Cariappa mentioned this pull request Nov 28, 2024

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

Merged

aaronj0 pushed a commit to compiler-research/cppyy that referenced this pull request Nov 28, 2024

[test] update tags

19303c2

fixed by compiler-research/cppyy-backend#115 & compiler-research/CppInterOp#351

resolve static constexpr class attribute #351

resolve static constexpr class attribute #351

Uh oh!

Conversation

Vipul-Cariappa commented Nov 12, 2024

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Vipul-Cariappa commented Nov 13, 2024

Uh oh!

vgvassilev commented Nov 13, 2024

Uh oh!

vgvassilev Nov 13, 2024

Choose a reason for hiding this comment

Uh oh!

Vipul-Cariappa Nov 13, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Vipul-Cariappa commented Nov 13, 2024

Uh oh!

vgvassilev Nov 13, 2024

Choose a reason for hiding this comment

Uh oh!

vgvassilev Nov 13, 2024

Choose a reason for hiding this comment

Uh oh!

Vipul-Cariappa Nov 13, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vgvassilev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

codecov bot commented Nov 12, 2024 •

edited

Loading