Skip to content

Conversation

@mcbarton
Copy link
Collaborator

Description

Please include a summary of changes, motivation and context for this PR.

The motivation for this PR is to offer a route to a smaller Emscripten shared library. If you build llvm without assertions at the moment you will get unused variable warnings in CppInterOp. This PR fixes these warnings. The reason for wanting CppInterOp to be able to be built without assertions, is that if you turn them off it has a noticeable effect on the size of the Emscripten shared library. The Oz and flto flags bring the size down to 44Mb. If you turn off assertions, then the size reduces further to 39Mb.

Fixes # (issue)

Type of change

Please tick all options which are relevant.

  • Bug fix
  • New feature
  • Requires documentation updates

Testing

Please describe the test(s) that you added and ran to verify your changes.

Checklist

  • I have read the contribution guide recently

@mcbarton mcbarton requested a review from vgvassilev July 18, 2025 14:09
@mcbarton
Copy link
Collaborator Author

This PR is ready for review. The motivation for wanting to be able to build CppInterOp with llvm assertions off is described in the PR description.

@github-actions
Copy link
Contributor

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

@mcbarton mcbarton force-pushed the Fix-unused-variable-warning-if-llvm-enable-assertions-off branch from 2ad2f93 to 8f5f3b8 Compare July 18, 2025 14:22
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!

@codecov
Copy link

codecov bot commented Jul 18, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.52%. Comparing base (ea8b2aa) to head (8f5f3b8).
⚠️ Report is 43 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #679   +/-   ##
=======================================
  Coverage   79.52%   79.52%           
=======================================
  Files           9        9           
  Lines        3917     3917           
=======================================
  Hits         3115     3115           
  Misses        802      802           
Files with missing lines Coverage Δ
lib/CppInterOp/DynamicLibraryManager.cpp 73.09% <ø> (ø)
lib/CppInterOp/DynamicLibraryManagerSymbol.cpp 68.01% <ø> (ø)
lib/CppInterOp/Paths.cpp 65.97% <ø> (ø)
Files with missing lines Coverage Δ
lib/CppInterOp/DynamicLibraryManager.cpp 73.09% <ø> (ø)
lib/CppInterOp/DynamicLibraryManagerSymbol.cpp 68.01% <ø> (ø)
lib/CppInterOp/Paths.cpp 65.97% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Contributor

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

@mcbarton mcbarton merged commit 82f08c6 into compiler-research:main Jul 18, 2025
57 of 58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants