Skip to content

Avoid fixed-size long integral types for macOS#449

Merged
wlav merged 1 commit intoFramework-R-D:mainfrom
knoepfel:python-macos-workaround
Mar 26, 2026
Merged

Avoid fixed-size long integral types for macOS#449
wlav merged 1 commit intoFramework-R-D:mainfrom
knoepfel:python-macos-workaround

Conversation

@knoepfel
Copy link
Copy Markdown
Member

As mentioned in a Slack discussion:

The fixed-width integer type std::int64_t is a typedef (https://en.cppreference.com/w/cpp/types/integer.html).   In particular, std::int64_t on macOS is a typedef to long long.  So there is a mismatch between the long that the Python system expects, and the long long that is calculated by the type_id system.

This PR is to work around the problem until we have consistent treatment of types between C++ and Python.

@greenc-FNAL
Copy link
Copy Markdown
Contributor

Review the full CodeQL report for details.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

@@           Coverage Diff           @@
##             main     #449   +/-   ##
=======================================
  Coverage   84.90%   84.90%           
=======================================
  Files         127      127           
  Lines        3339     3339           
  Branches      575      575           
=======================================
  Hits         2835     2835           
  Misses        308      308           
  Partials      196      196           
Flag Coverage Δ
unittests 84.90% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
plugins/python/src/modulewrap.cpp 76.69% <ø> (ø)

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 547c196...d7eca9a. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@beojan
Copy link
Copy Markdown
Contributor

beojan commented Mar 24, 2026

This seems like it might be too restrictive actually, there might be other platform / libc combinations where int64_t isn't long.

In fact, if for some crazy reason someone's using a 32-bit platform, int64_t would be long long.

@beojan
Copy link
Copy Markdown
Contributor

beojan commented Mar 25, 2026

Thinking further, the generalization would be something like

if int64_t === long
    BASIC_CONVERTER(long, int64_t /* = long */...
else
    BASIC_CONVERTER(long, long ...
end

and then we might as well just have the versions without fixed-width types.

@knoepfel
Copy link
Copy Markdown
Member Author

This seems like it might be too restrictive actually, there might be other platform / libc combinations where int64_t isn't long.

In fact, if for some crazy reason someone's using a 32-bit platform, int64_t would be long long.

Unconditionally requiring long to be int64_t is the over-restriction. This PR relaxes that restriction for macOS.

I agree there are other systems for which long and int64_t are not synonymous. This PR, however, is strictly a workaround: to enable continued Phlex development on macOS, not to support systems that are not currently used in our development, CI, or deployment.

A long-term solution will be the subject of another PR.

@knoepfel knoepfel force-pushed the python-macos-workaround branch from 264b31a to d7eca9a Compare March 26, 2026 13:36
@knoepfel knoepfel requested review from beojan, greenc-FNAL and wlav March 26, 2026 13:45
Copy link
Copy Markdown
Contributor

@wlav wlav left a comment

Choose a reason for hiding this comment

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

As discussed, fine for now. I'll reconsider when dealing with the refactoring and/or pull in the converters from a different place (pybind11 or cppyy) once we go to real data products.

@wlav wlav merged commit 06b11e3 into Framework-R-D:main Mar 26, 2026
35 of 36 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.

4 participants