Complete nogil support for all converters #246
Conversation
This requires a change in conversion providers. +Some other cleanups.
Update all remaining TypeConverter classes to return the 4-tuple (code, call_as, cleanup, decl) from input_conversion() and add with_cdef parameter to call_method() for nogil compatibility. Converters updated: - StdVectorAsNumpyConverter - StdUnorderedMapConverter - StdUnorderedSetConverter - StdDequeConverter - StdListConverter - StdOptionalConverter - StdStringViewConverter Also update environment.yml to require Cython>=3.1 for nogil support. Co-Authored-By: Claude Opus 4.5 <[email protected]>
📝 WalkthroughWalkthroughThis PR refactors the code generation pipeline to support declaration splitting for nogil contexts by extending Changes
Sequence Diagram(s)sequenceDiagram
participant Conv as TypeConverter
participant CG as CodeGenerator
participant Out as Output Code
Note over Conv,Out: Extended Declaration Handling (New Flow)
Conv->>CG: input_conversion() returns<br/>(code, call_as, cleanup, decl)
alt With nogil && has declarations
CG->>Out: emit init block with decls
CG->>Out: emit with nogil context
CG->>Out: use decl_calls instead of call_args<br/>inside nogil block
else Without nogil
CG->>Out: emit code + call_args directly<br/>as before
end
Out-->>Conv: Generated C/Cython code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
autowrap/ConversionProvider.py (5)
2274-2276: Type annotation doesn't match actual return type.The
input_conversiontype annotation showsTuple[Code, str, Union[Code, str]](3 elements), but the actual returns at lines 2314-2315 and 2343-2344 include a 4thdeclelement. The annotation should beTuple[Code, str, Union[Code, str], Tuple[str, str]].🔧 Suggested fix
def input_conversion( self, cpp_type: CppType, argument_var: str, arg_num: int - ) -> Tuple[Code, str, Union[Code, str]]: + ) -> Tuple[Code, str, Union[Code, str], Tuple[str, str]]:
2773-2775: Type annotation missingdecltuple element.Similar to
StdVectorAsNumpyConverter, the return type annotation is missing the 4thTuple[str, str]element fordecl. The actual return at lines 2912-2913 correctly includes it.🔧 Suggested fix
def input_conversion( self, cpp_type: CppType, argument_var: str, arg_num: int - ) -> Tuple[Code, str, Union[Code, str]]: + ) -> Tuple[Code, str, Union[Code, str], Tuple[str, str]]:
3067-3069: Type annotation missingdecltuple element inStdUnorderedSetConverter.Same issue - annotation shows 3-tuple but returns 4-tuple.
3278-3280: Type annotation missingdecltuple element inStdDequeConverter.Same issue - annotation shows 3-tuple but returns 4-tuple.
3445-3447: Type annotation missingdecltuple element inStdListConverter.Same issue - annotation shows 3-tuple but returns 4-tuple.
autowrap/CodeGenerator.py (1)
1456-1480: Multi-lineinitblock loses indentation for declarations after the first.When a nogil method has multiple arguments,
initcontains multiple lines (one per declaration). The template| $initonly applies the 4-space indentation to the first line. Subsequent lines from the\nininitwill start at column 0, producing invalid Cython code.Example with two arguments:
# init = "cdef int c_x = (<int>x)\ncdef int c_y = (<int>y)\n" # Results in: cdef int c_x = (<int>x) cdef int c_y = (<int>y) # <-- Wrong: no indentation with nogil:🔧 Proposed fix: Pre-indent each declaration line
init = "" c_call_args = [] for decl, decl_call in zip(decls, decl_calls): - init = init + decl + "\n" + init = init + " " + decl + "\n" c_call_args.append(decl_call) # call wrapped method and convert result value back to python cpp_name = method.cpp_decl.name if method.with_nogil and init.strip("\n"): call_args = c_call_args call_args_str = ", ".join(call_args) ... if method.with_nogil: meth_code.add( """ - | $init + |$init | with nogil: | """, locals() )Alternatively, build each declaration with the proper indentation at the source, or emit declarations individually via
meth_code.add()calls.
🤖 Fix all issues with AI agents
In `@autowrap/ConversionProvider.py`:
- Around line 200-201: The VoidConverter.init_as_call_arg currently returns a
libcpp_string assignment (copied from StdStringConverter) which is incorrect for
void; update VoidConverter.init_as_call_arg to not emit that string and instead
raise NotImplementedError (or return an empty string if you prefer base-class
consistency) so void types cannot produce bogus call-arg initialization; edit
the method on the VoidConverter class (init_as_call_arg) to either raise
NotImplementedError("init_as_call_arg not applicable for void") or return "".
In `@environment.yml`:
- Line 5: Update the cython dependency entry "cython>=3.1" in environment.yml to
either pin a safe version and/or document the experimental nogil status: replace
with a pinned spec like "cython==3.1.0" (or "cython>=3.1,<3.2") if you want
reproducible installs, and add a short inline comment or adjacent README note
stating that Cython 3.1's nogil/free-threading is experimental and may
re-acquire the GIL for exceptions or Python object access so the team can review
limitations before production use.
🧹 Nitpick comments (2)
environment.yml (1)
4-6: Consider specifying a Python version constraint.The environment configuration lacks a Python version specification, which could lead to compatibility issues across different Python versions. Consider adding a Python version constraint to ensure consistent behavior.
🐍 Proposed addition for Python version
dependencies: + - python>=3.8 - cython>=3.1 - pytestNote: Adjust the Python version as appropriate for your project's requirements.
tests/test_files/gil_testing.hpp (1)
53-57: Python 2.0 code is dead code.Python 2 has been EOL since January 2020. The
#elsebranch for Python 2.0 (lines 54-57) will never be compiled when using Python 3. Consider removing this dead code for maintainability.This applies to both
do_something(lines 24-28) anddo_something2(lines 53-57).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CONTRIBUTING.mdautowrap/CodeGenerator.pyautowrap/ConversionProvider.pyautowrap/DeclResolver.pyautowrap/PXDParser.pyautowrap/data_files/boost/cstdint.hppenvironment.ymlrequirements_dev.txtsetup.cfgtests/test_code_generator.pytests/test_code_generator_minimal.pytests/test_files/converters/IntHolderConverter.pytests/test_files/gil_testing.hpptests/test_files/gil_testing.pxdtests/test_files/libcpp_stl_test.pxd
💤 Files with no reviewable changes (2)
- setup.cfg
- requirements_dev.txt
🧰 Additional context used
🧬 Code graph analysis (5)
autowrap/DeclResolver.py (1)
autowrap/Types.py (1)
CppType(42-289)
autowrap/PXDParser.py (1)
autowrap/Code.py (1)
Code(41-90)
tests/test_files/converters/IntHolderConverter.py (1)
autowrap/Code.py (2)
Code(41-90)add(56-75)
autowrap/ConversionProvider.py (3)
autowrap/Types.py (2)
CppType(42-289)toString(168-200)autowrap/Code.py (2)
Code(41-90)add(56-75)tests/test_files/converters/IntHolderConverter.py (2)
matches(9-10)input_conversion(18-32)
autowrap/CodeGenerator.py (1)
autowrap/ConversionProvider.py (16)
input_conversion(148-161)input_conversion(218-221)input_conversion(258-263)input_conversion(294-299)input_conversion(338-345)input_conversion(372-379)input_conversion(403-410)input_conversion(472-484)input_conversion(521-528)input_conversion(558-567)input_conversion(597-604)input_conversion(644-655)input_conversion(802-870)input_conversion(980-1228)input_conversion(1368-1470)input_conversion(1802-2046)
🪛 Clang (14.0.6)
autowrap/data_files/boost/cstdint.hpp
[error] 36-36: 'boost/config.hpp' file not found
(clang-diagnostic-error)
🪛 markdownlint-cli2 (0.18.1)
CONTRIBUTING.md
34-34: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🪛 Ruff (0.14.11)
autowrap/ConversionProvider.py
60-60: Unused method argument: arg_num
(ARG002)
85-85: Unused method argument: res_type
(ARG002)
85-85: Unused method argument: with_const
(ARG002)
200-200: Unused method argument: cpp_type
(ARG002)
200-200: Unused method argument: arg_num
(ARG002)
206-206: Unused method argument: res_type
(ARG002)
206-206: Unused method argument: with_const
(ARG002)
206-206: Unused method argument: with_cdef
(ARG002)
258-258: Unused method argument: arg_num
(ARG002)
294-294: Unused method argument: arg_num
(ARG002)
530-530: Unused method argument: res_type
(ARG002)
530-530: Unused method argument: with_const
(ARG002)
569-569: Unused method argument: res_type
(ARG002)
569-569: Unused method argument: with_const
(ARG002)
606-606: Unused method argument: res_type
(ARG002)
606-606: Unused method argument: with_const
(ARG002)
622-622: Unused method argument: arg_num
(ARG002)
873-873: Unused method argument: res_type
(ARG002)
873-873: Unused method argument: with_const
(ARG002)
877-877: Unused method argument: res_type
(ARG002)
877-877: Unused method argument: with_const
(ARG002)
877-877: Unused method argument: with_cdef
(ARG002)
1231-1231: Unused method argument: res_type
(ARG002)
1231-1231: Unused method argument: with_const
(ARG002)
1235-1235: Unused method argument: res_type
(ARG002)
1235-1235: Unused method argument: with_const
(ARG002)
1235-1235: Unused method argument: with_cdef
(ARG002)
1473-1473: Unused method argument: res_type
(ARG002)
1473-1473: Unused method argument: with_const
(ARG002)
1477-1477: Unused method argument: res_type
(ARG002)
1477-1477: Unused method argument: with_const
(ARG002)
1477-1477: Unused method argument: with_cdef
(ARG002)
2049-2049: Unused method argument: with_const
(ARG002)
2057-2057: Unused method argument: with_const
(ARG002)
2057-2057: Unused method argument: with_cdef
(ARG002)
2545-2545: Unused method argument: cpp_type
(ARG002)
2545-2545: Unused method argument: arg_num
(ARG002)
2915-2915: Unused method argument: res_type
(ARG002)
2915-2915: Unused method argument: with_const
(ARG002)
2915-2915: Unused method argument: with_cdef
(ARG002)
3171-3171: Unused method argument: res_type
(ARG002)
3171-3171: Unused method argument: with_const
(ARG002)
3171-3171: Unused method argument: with_cdef
(ARG002)
3349-3349: Unused method argument: res_type
(ARG002)
3349-3349: Unused method argument: with_const
(ARG002)
3349-3349: Unused method argument: with_cdef
(ARG002)
3522-3522: Unused method argument: res_type
(ARG002)
3522-3522: Unused method argument: with_const
(ARG002)
3522-3522: Unused method argument: with_cdef
(ARG002)
3654-3654: Unused method argument: res_type
(ARG002)
3654-3654: Unused method argument: with_const
(ARG002)
3654-3654: Unused method argument: with_cdef
(ARG002)
3745-3745: Unused method argument: res_type
(ARG002)
3745-3745: Unused method argument: with_const
(ARG002)
3745-3745: Unused method argument: with_cdef
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test (==3.2.0, 3.10)
- GitHub Check: test (==3.2.0, 3.13)
- GitHub Check: test (==3.2.0, 3.12)
- GitHub Check: test (==3.1.0, 3.11)
- GitHub Check: test (==3.1.0, 3.10)
- GitHub Check: test (==3.1.0, 3.13)
- GitHub Check: test (==3.2.0, 3.11)
- GitHub Check: test (==3.1.0, 3.12)
🔇 Additional comments (29)
environment.yml (1)
1-3: LGTM!The environment name and channel configuration are appropriate for this project.
CONTRIBUTING.md (2)
7-17: LGTM! Helpful dev environment setup instructions.The new section provides a clear, concise workflow for contributors to get started.
34-37: Documentation improvements look good.The typo corrections ("meber"→"member", "github"→"GitHub", "repoduce"→"reproduce") improve readability.
Note: Static analysis flags MD007 (list indentation) on these lines, but the 2-space indent is consistent with the existing list style in lines 29-33.
tests/test_files/gil_testing.pxd (1)
3-3: LGTM! Proper extension of GIL testing infrastructure.The new
do_something2method tests nogil behavior withlibcpp_stringparameters (vs.const_char*in the original method), which provides good coverage for the string conversion path in nogil contexts.Also applies to: 10-10
tests/test_code_generator.py (1)
326-327: LGTM! Good test coverage for the new nogil method.The test correctly exercises
do_something2with a string argument and verifies the expected behavior, providing coverage for nogil string conversions.autowrap/DeclResolver.py (1)
231-231: LGTM! Type annotation improves code clarity.The explicit
Types.CppTypeannotation aligns with other typed attributes inResolvedMethodand supports better IDE/type-checker integration.tests/test_files/converters/IntHolderConverter.py (1)
18-32: LGTM! Correctly updated to the new 4-tuple API.The changes properly align with the PR's
input_conversion()API change:
- Fixed typo in comment ("behavoir" → "behavior")
- Added
decl = ("", call_as)tuple where the empty string indicates the declaration is already embedded incode- Returns the new 4-tuple
(code, call_as, cleanup, decl)This pattern is appropriate since the
cdef _Holder[int]declaration is handled inline within the code block.tests/test_code_generator_minimal.py (1)
69-76: LGTM - Test converter correctly updated for new 4-tuple return.The
input_conversionmethod now returns the expected 4-tuple(code, call_as, cleanup, decl)wheredeclis("cdef int c_<var> = <call_as>", "c_<var>"). This aligns with the PR's goal of supporting split declarations for nogil contexts.tests/test_files/gil_testing.hpp (1)
41-68: Newdo_something2method provides std::string parameter test coverage.The method correctly tests GIL release behavior with
const std::string¶meter type, complementing the existingconst char*test. The GIL-check logic is appropriate for verifying that the GIL is released before entering C++ code.tests/test_files/libcpp_stl_test.pxd (1)
51-53: LGTM - Test declarations correctly add nogil qualifiers.The
nogilqualifiers onprocess_8_mapandprocess_9_mapprovide test coverage for:
- Returning
libcpp_map[IntWrapper, int]with nogil (line 51)- Accepting
libcpp_map[int, IntWrapper]&with nogil (line 53)This validates the extended input_conversion and call_method handling for map types in nogil contexts.
autowrap/PXDParser.py (2)
57-57: LGTM - Type annotation correctly expanded to includeCode.The
AnnotDicttype alias now accurately reflects that values can beCodeobjects (used forwrap-docannotations), not justboolorList[str].
83-84: Good documentation of technical debt.The TODO comment acknowledges the inconsistency between
CodeandList[str]value types. This helps future maintainers understand the design decision.autowrap/ConversionProvider.py (8)
56-64: LGTM - Base class extended with nogil support infrastructure.The
__init__method properly initializesenums_to_wrapandinit_as_call_argprovides a sensible default for generating C variable declarations. The patterncdef <type> c_<var> = <var>is appropriate for split declarations.
84-123: Well-designedcall_method_splitandwith_cdefextension.The
call_method_splitmethod returns a tuple of(declaration, assignment)enabling declarations to be placed beforewith nogil:blocks while assignments occur inside. Thewith_cdefparameter incall_methodprovides backward compatibility while supporting nogil contexts.The detailed TODO comment (lines 89-96) is excellent documentation explaining the rationale and potential future improvements.
148-161: Clear documentation of the new 4-tuple return format.The updated docstring correctly documents the
decltuple as containing(declaration, call_expression)for split-declaration paths, primarily used in nogil contexts.
258-263: Consistent 4-tuple pattern inIntegerConverter.The
input_conversioncorrectly returns the new format withdecl = ("cdef <type> c_<var> = <call_as>", "c_<var>"). This pattern is consistently applied across all primitive type converters.
869-870: Correctdeclhandling for already-declared variables.When the declaration is already included in the conversion code, returning
("", temp_var)fordeclcorrectly indicates no additional declaration is needed while still providing the call expression. This pattern is consistently applied across container converters.
3763-3786: LGTM - CleanerConverterRegistryinitialization lifecycle.The
instance_mappingis now properly initialized once in__init__(line 3764) and populated inprocess_and_set_type_mapping(lines 3784-3786). This is a cleaner pattern than potentially reinitializing the dict.
1160-1160: Minor style inconsistency in condition.Line 1160 uses
not inwhile similar conditions elsewhere use consistent spacing. This is a nitpick.and tt_key.base_type not in self.converters.names_of_wrapper_classesvs the expected pattern from other similar lines that use the same format. No action needed.
1695-1733:TypeToWrapConverter.call_methodcorrectly handleswith_cdeffor wrapped types.The method properly generates code with or without
cdefdeclarations based on thewith_cdefparameter. For pointer returns with null checking, both paths are handled correctly (lines 709-728).autowrap/CodeGenerator.py (9)
1199-1216: LGTM!The initialization and population of
declsanddecl_callslists correctly mirrors the existing pattern forcall_argsandcleanups. The tuple unpacking aligns with the extendedinput_conversion()return signature.
1430-1441: LGTM on the conditional logic.The condition at line 1439 correctly ensures
call_argsis only replaced withc_call_argswhen there are actual declarations to emit. Theinit.strip("\n")check handles edge cases where decls might be empty strings.
1305-1305: LGTM!The extended return tuple correctly provides the declaration info needed for nogil context handling.
1454-1454: LGTM!Setting
with_cdef=not method.with_nogilcorrectly preventscdefstatements inside thewith nogil:block, where they're syntactically invalid.
1663-1665: Consistent API unpacking for constructors.The extended tuple is correctly unpacked for API consistency, even though constructors don't use nogil declarations.
1920-1920: LGTM!Correctly unpacks the extended tuple from
input_conversion()for consistency with the updated API.
1322-1322: LGTM!Consistent API handling for the attribute wrapper.
1417-1428: LGTM!The method signature correctly adds the
ResolvedMethodtype annotation, and the extended tuple unpacking aligns with the updated return from_create_fun_decl_and_input_conversion.
1549-1564: Clarify: Free functions don't supportwith_nogilbut unpacking suggests API supports it.In
_create_wrapper_for_free_function, thedeclsanddecl_callsvariables are unpacked from_create_fun_decl_and_input_conversionbut never used, unlike increate_wrapper_for_nonoverloaded_method(lines 1432-1434) where they initialize the call arguments.Additionally, free functions don't check
method.with_nogiland thecall_methodcall at line 1564 uses default parameters, whereas class methods explicitly passwith_cdef = not method.with_nogil(line 1454) to handle nogil contexts.This inconsistency suggests either:
- Free functions intentionally don't support nogil (in which case the unpacking can be removed to match the API contract), or
- Free function nogil support is incomplete and would require similar handling to class methods
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def init_as_call_arg(self, cpp_type, argument_var, arg_num): | ||
| return "cdef libcpp_string c_%s = %s" % (argument_var, argument_var) |
There was a problem hiding this comment.
Suspicious init_as_call_arg implementation in VoidConverter.
The VoidConverter.init_as_call_arg returns "cdef libcpp_string c_%s = %s" which seems incorrect for a void type. This appears to be copy-pasted from StdStringConverter (line 2545-2546). While init_as_call_arg may never be called for void types in practice, this could lead to confusing errors if it ever is invoked.
Consider either:
- Raising
NotImplementedErrorlike other void-related methods - Returning an empty string consistent with the base class pattern
🔧 Suggested fix
def init_as_call_arg(self, cpp_type, argument_var, arg_num):
- return "cdef libcpp_string c_%s = %s" % (argument_var, argument_var)
+ raise NotImplementedError("void has no call argument")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def init_as_call_arg(self, cpp_type, argument_var, arg_num): | |
| return "cdef libcpp_string c_%s = %s" % (argument_var, argument_var) | |
| def init_as_call_arg(self, cpp_type, argument_var, arg_num): | |
| raise NotImplementedError("void has no call argument") |
🧰 Tools
🪛 Ruff (0.14.11)
200-200: Unused method argument: cpp_type
(ARG002)
200-200: Unused method argument: arg_num
(ARG002)
🤖 Prompt for AI Agents
In `@autowrap/ConversionProvider.py` around lines 200 - 201, The
VoidConverter.init_as_call_arg currently returns a libcpp_string assignment
(copied from StdStringConverter) which is incorrect for void; update
VoidConverter.init_as_call_arg to not emit that string and instead raise
NotImplementedError (or return an empty string if you prefer base-class
consistency) so void types cannot produce bogus call-arg initialization; edit
the method on the VoidConverter class (init_as_call_arg) to either raise
NotImplementedError("init_as_call_arg not applicable for void") or return "".
| // boost cstdint.hpp header file ------------------------------------------// | ||
|
|
||
| // (C) Copyright Beman Dawes 1999. | ||
| // (C) Copyright Jens Mauer 2001 | ||
| // (C) Copyright John Maddock 2001 | ||
| // Distributed under the Boost | ||
| // Software License, Version 1.0. (See accompanying file | ||
| // LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt) | ||
|
|
||
| // See http://www.boost.org/libs/integer for documentation. | ||
|
|
||
| // Revision History | ||
| // 31 Oct 01 use BOOST_HAS_LONG_LONG to check for "long long" (Jens M.) | ||
| // 16 Apr 01 check LONGLONG_MAX when looking for "long long" (Jens Maurer) | ||
| // 23 Jan 01 prefer "long" over "int" for int32_t and intmax_t (Jens Maurer) | ||
| // 12 Nov 00 Merged <boost/stdint.h> (Jens Maurer) | ||
| // 23 Sep 00 Added INTXX_C macro support (John Maddock). | ||
| // 22 Sep 00 Better 64-bit support (John Maddock) | ||
| // 29 Jun 00 Reimplement to avoid including stdint.h within namespace boost | ||
| // 8 Aug 99 Initial version (Beman Dawes) | ||
|
|
||
|
|
||
| #ifndef BOOST_CSTDINT_HPP | ||
| #define BOOST_CSTDINT_HPP |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the cstdint.hpp file to see the actual includes and dependencies
cat -n autowrap/data_files/boost/cstdint.hpp | head -50Repository: OpenMS/autowrap
Length of output: 2189
🏁 Script executed:
# Check for the includes mentioned at lines 36 and 204
sed -n '30,40p' autowrap/data_files/boost/cstdint.hpp
echo "---"
sed -n '200,210p' autowrap/data_files/boost/cstdint.hppRepository: OpenMS/autowrap
Length of output: 560
🏁 Script executed:
# Check if boost/config.hpp and boost/limits.hpp exist in the repository
find autowrap -name "config.hpp" -o -name "limits.hpp" | head -20Repository: OpenMS/autowrap
Length of output: 41
🏁 Script executed:
# Check the entire directory structure of the vendored boost files
ls -la autowrap/data_files/boost/ 2>/dev/null | head -30Repository: OpenMS/autowrap
Length of output: 234
🌐 Web query:
C++11 cstdint header vs boost cstdint differences capabilities
💡 Result:
Short comparison — main differences and capabilities
-
Standard vs Boost: is the C++ standard header (types in namespace std/global); <boost/cstdint.hpp> is Boost’s compatibility header (typedefs in namespace boost) intended for pre‑C++11 portability. [1] (manuals.quinngrier.com)
-
Availability on old compilers: Boost was written to provide stdint-like typedefs on older compilers that lacked <stdint.h>/; it may supply only the types the platform supports and avoid placing names in std. Use Boost when targeting pre‑C++11 toolchains. [1] (boost.org)
-
Exact contents: Both expose fixed-width (intN_t/uintN_t), least-/fast- types, intptr_t/uintptr_t, and intmax_t when supported — but is governed by the C/C++ standard rules (some types are optional if the implementation lacks that width). Boost mirrors the C99 <stdint.h> semantics but places names in boost::. [2][1] (manuals.quinngrier.com)
-
Additional Boost features: Boost.Integer offers related utilities (boost::integer_traits, type-selection helpers, safer/portable wrappers) beyond just typedefs; <boost/cstdint.hpp> is part of that ecosystem. provides only the types and the C-format macros (from C99). [1] (cs.brown.edu)
-
Portability/caveats:
- is the standard choice for modern C++ (C++11 onward). Use it when targeting C++11+ compilers. [2] (manuals.quinngrier.com)
- <boost/cstdint.hpp> may expose some C macro names as implementation artifacts and keeps all names in boost:: to avoid polluting std/global; it can be redundant on modern toolchains but remains useful for legacy portability. [1] (boost.org)
If you want, I can list concrete symbols provided by each header on a given compiler (e.g., GCC/Clang/MSVC) — tell me which compiler and version.
Citations:
- 1: https://manuals.quinngrier.com/cppreference/20121202/output/en/cpp/types/integer.html?utm_source=openai
- 2: https://www.boost.org/doc/libs/1_80_0/libs/config/doc/html/boost_config/cstdint.html?utm_source=openai
- 3: https://manuals.quinngrier.com/cppreference/20121202/output/en/cpp/types/integer.html?utm_source=openai
- 4: https://cs.brown.edu/~jwicks/boost/libs/integer/index.html?utm_source=openai
- 5: https://manuals.quinngrier.com/cppreference/20121202/output/en/cpp/types/integer.html?utm_source=openai
- 6: https://www.boost.org/doc/libs/1_80_0/libs/config/doc/html/boost_config/cstdint.html?utm_source=openai
🏁 Script executed:
# Check for C++ standard in CMakeLists.txt
rg -i "cxx_standard|c\+\+11|c\+\+14|c\+\+17|c\+\+20" --max-count 20Repository: OpenMS/autowrap
Length of output: 3194
🏁 Script executed:
# Check if there are any references to this boost cstdint header in the code
rg "boost/cstdint|boost::int8_t|boost::uint" --type cpp --type hpp | head -20Repository: OpenMS/autowrap
Length of output: 86
🏁 Script executed:
# Get the full size and context of the file to understand its actual usage
wc -l autowrap/data_files/boost/cstdint.hppRepository: OpenMS/autowrap
Length of output: 100
🏁 Script executed:
# Check if there are compilation attempts or test files that would indicate this is used
fd -e "test" -e "cmake" autowrap/data_files/boost/ 2>/dev/nullRepository: OpenMS/autowrap
Length of output: 41
Remove this incomplete vendored Boost header or ensure all transitive dependencies are included.
This file includes <boost/config.hpp> and <boost/limits.hpp> which are missing from the repository, making it non-functional. Since the project targets C++17 (per HISTORY.md and Utils.py), the standard <cstdint> header provides all necessary fixed-width integer types (int8_t, uint32_t, etc.) without external dependencies. Consider either:
- Removing this file if not actively used
- Using
<cstdint>directly (available in C++11 and later) - Including the complete Boost vendoring with all transitive dependencies
| # define __STDC_CONSTANT_MACROS | ||
| #endif | ||
|
|
||
| #include <boost/config.hpp> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for boost/config.hpp and boost/limits.hpp dependencies
echo "Searching for boost/config.hpp..."
fd -t f "config.hpp" | grep -i boost || echo "Not found"
echo ""
echo "Searching for boost/limits.hpp..."
fd -t f "limits.hpp" | grep -i boost || echo "Not found"
echo ""
echo "Checking autowrap/data_files/boost directory contents..."
fd . autowrap/data_files/boost 2>/dev/null | head -20 || echo "Directory not found or empty"Repository: OpenMS/autowrap
Length of output: 243
🏁 Script executed:
cat autowrap/data_files/boost/cstdint.hpp | head -100Repository: OpenMS/autowrap
Length of output: 3552
🏁 Script executed:
# Check for usage of BOOST_ macros that would come from config.hpp
rg "BOOST_[A-Z_]+" autowrap/data_files/boost/cstdint.hpp | head -30Repository: OpenMS/autowrap
Length of output: 1278
🏁 Script executed:
rg "limits.hpp" autowrap/data_files/boost/cstdint.hppRepository: OpenMS/autowrap
Length of output: 137
Missing Boost dependencies will prevent compilation.
This header includes boost/config.hpp (line 36) and boost/limits.hpp (line 204), both of which are not present in the repository. The file depends on macros from boost/config.hpp (BOOST_HAS_STDINT_H, BOOST_HAS_LONG_LONG, BOOST_MSVC, BOOST_HAS_MS_INT64, BOOST_WINDOWS, BOOST_NO_INT64_T) for conditional compilation. Without these dependencies, the file will fail to compile.
🧰 Tools
🪛 Clang (14.0.6)
[error] 36-36: 'boost/config.hpp' file not found
(clang-diagnostic-error)
| channels: | ||
| - conda-forge | ||
| dependencies: | ||
| - cython>=3.1 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Cython 3.1 release date stable version
💡 Result:
Cython 3.1.0 (stable) was released on May 8, 2025. [1][2]
Sources:
- piwheels release listing. [1]
- SourceForge / PyPI release files showing 3.1.0 uploaded May 8, 2025. [2]
🌐 Web query:
Cython 3.1 nogil support compatibility
💡 Result:
Short summary — Cython 3.1 nogil compatibility
- Cython 3.1 adds basic/experimental support for Python’s free-threaded (nogil) build (the “free-threaded” mode introduced around Python 3.13). Support is explicitly labelled experimental and may have known pitfalls. (cython.readthedocs.io)
- You can use nogil as a context manager or decorator in pure-Python mode (with cython.nogil / with cython.nogil) and mark cfuncs as nogil; Cython also provides cython.gil/with_gil helpers. (cython.readthedocs.io)
- Cython will still re-acquire the GIL when needed (e.g., exceptions, Python object access), and some behaviors (thread-state handling, extension-type safety, and edge cases) remain delicate — read the “Free threading” notes before using in production. (docs.cython.org)
If you want, I can fetch the exact 3.1.x release notes or copy the relevant “Free threading” section for closer reading.
Citations:
- 1: https://cython.readthedocs.io/en/3.1.x/src/userguide/freethreading.html?utm_source=openai
- 2: https://cython.readthedocs.io/en/3.1.x/src/tutorial/pure.html?utm_source=openai
- 3: https://docs.cython.org/en/latest/src/userguide/nogil.html?utm_source=openai
Note that Cython 3.1's nogil support is experimental.
Cython 3.1.0 (released May 8, 2025) is stable and available, and it does include nogil/free-threading support. However, this support is explicitly marked as experimental with known limitations and pitfalls. The implementation will re-acquire the GIL when needed (e.g., for exceptions or Python object access). Consider documenting this experimental status or reviewing the free-threading limitations before use in production.
🤖 Prompt for AI Agents
In `@environment.yml` at line 5, Update the cython dependency entry "cython>=3.1"
in environment.yml to either pin a safe version and/or document the experimental
nogil status: replace with a pinned spec like "cython==3.1.0" (or
"cython>=3.1,<3.2") if you want reproducible installs, and add a short inline
comment or adjacent README note stating that Cython 3.1's nogil/free-threading
is experimental and may re-acquire the GIL for exceptions or Python object
access so the team can review limitations before production use.
Summary
This PR completes the
nogilsupport for autowrap-generated code, building on PR #181. When a method is marked withnogilin the PXD file (e.g.,void process() nogil), the generated wrapper will release Python's Global Interpreter Lock (GIL) before calling into C++, allowing true parallelism.Key Changes
input_conversion()now returns a 4-tuple(code, call_as, cleanup, decl)wheredeclis a tuple(declaration, call_expression)for split declaration in nogil contextscall_method()signature: Addedwith_cdefparameter to control whether to includecdef(not allowed insidenogil:blocks)>=3.1for improved nogil supportUpdated Converters
All TypeConverter classes now support nogil:
nogil + OpenMP Thread Interaction Analysis
When a
nogil-marked method (e.g., in pyOpenMS) internally uses OpenMP for parallelism:How It Works
with nogil:blockSafety Considerations
with gil:to reacquire GIL firstExample Flow
Bottom Line
This is safe and beneficial for libraries like OpenMS that use OpenMP internally:
Test Plan
gil_testing.hppverifies GIL is properly releasedReferences
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.