Skip to content

Fix assert for external interpreter #367

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 1 commit into from
Dec 6, 2024
Merged

Fix assert for external interpreter #367

merged 1 commit into from
Dec 6, 2024

Conversation

aaronj0
Copy link
Collaborator

@aaronj0 aaronj0 commented Dec 3, 2024

No description provided.

Copy link
Contributor

github-actions bot commented Dec 3, 2024

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

Copy link
Contributor

github-actions bot commented Dec 3, 2024

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

Copy link
Contributor

github-actions bot commented Dec 3, 2024

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

Copy link
Contributor

github-actions bot commented Dec 3, 2024

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

Copy link

codecov bot commented Dec 3, 2024

Codecov Report

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

Project coverage is 76.15%. Comparing base (74ad637) to head (a70fcc7).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
lib/Interpreter/CppInterOp.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #367      +/-   ##
==========================================
- Coverage   76.31%   76.15%   -0.16%     
==========================================
  Files           8        8              
  Lines        3137     3137              
==========================================
- Hits         2394     2389       -5     
- Misses        743      748       +5     
Files with missing lines Coverage Δ
lib/Interpreter/CppInterOp.cpp 79.80% <0.00%> (-0.28%) ⬇️
Files with missing lines Coverage Δ
lib/Interpreter/CppInterOp.cpp 79.80% <0.00%> (-0.28%) ⬇️

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

@aaronj0 aaronj0 force-pushed the fix-assert branch 3 times, most recently from 6309657 to 12d45fc Compare December 3, 2024 21:42

#if !defined(NDEBUG) || !GTEST_HAS_DEATH_TEST
// death tests don't work right on windows
#ifndef _WIN32
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this was covered by GTEST_HAS_DEATH_TEST. Did anything in particular fail?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it doesn't hit the assertion:
1: Actual msg: 1: [ DEATH ] Unable to find target for this triple (no targets are registered)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shouldn't we do #if !defined(NDEBUG) && !GTEST_HAS_DEATH_TEST?

Copy link
Contributor

Choose a reason for hiding this comment

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

We do that, we do not need the _WIN32 thing...

Yes, it doesn't hit the assertion:
1: Actual msg: 1: [ DEATH ] Unable to find target for this triple (no targets are registered)

Seems a different problem...

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!

@aaronj0 aaronj0 merged commit b90a9db into main Dec 6, 2024
42 of 44 checks passed
# 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