Skip to content

Conversation

@minggangw
Copy link
Member

This patch implements to enable C++20 only when nodejs >= 23, which improve the compatibility with g++ lower versions, because C++20 only needed for nodejs >= 23.

Fix: #1089

@minggangw minggangw requested a review from Copilot March 31, 2025 05:14
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the build configuration to conditionally enable C++20 only when Node.js is version 23 or higher, while using C++17 for lower versions.

  • Introduces a new key to determine the major Node.js version.
  • Applies conditional logic in the build flags for both Linux and Windows targets based on the Node.js version.
Comments suppressed due to low confidence (1)

binding.gyp:92

  • Similarly, under the Windows section, 'cflags' is used for node_major_version >= 23 while 'cflags_cc' is used for node_major_version < 23. Using consistent keys could avoid unexpected behavior.
'cflags': [

binding.gyp Outdated
'conditions': [
[
'node_major_version >= 23', {
'cflags': [
Copy link

Copilot AI Mar 31, 2025

Choose a reason for hiding this comment

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

The use of 'cflags' under the condition for node_major_version >= 23 is inconsistent with the use of 'cflags_cc' for node_major_version < 23. Consider standardizing the flag key (likely using 'cflags_cc') to prevent potential build issues.

Suggested change
'cflags': [
'cflags_cc': [

Copilot uses AI. Check for mistakes.
@minggangw minggangw requested a review from Copilot March 31, 2025 05:19
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enables the use of C++20 only when Node.js version is 23 or above while falling back to C++17 otherwise, improving compatibility with older g++ versions.

  • Introduces a new variable, node_major_version, extracted from Node.js version.
  • Adds conditional blocks in both Linux and Windows targets to set the C++ standard appropriately.
Comments suppressed due to low confidence (1)

binding.gyp:15

  • The node_major_version value is extracted as a string, which may cause unexpected behavior when used in numeric comparisons. Consider converting it to a number to ensure accurate condition evaluation.
    'node_major_version': '<!(node -p "process.versions.node.split('.')[0]")',

@coveralls
Copy link

coveralls commented Mar 31, 2025

Coverage Status

coverage: 85.355%. remained the same
when pulling 6d79778 on minggangw:fix-1089
into ff07c86 on RobotWebTools:develop.

@minggangw minggangw merged commit 4e6f412 into RobotWebTools:develop Mar 31, 2025
10 of 11 checks passed
minggangw added a commit that referenced this pull request Mar 31, 2025
This patch implements to enable C++20 only when nodejs >= 23, which improve the compatibility with g++ lower versions, because C++20 only needed for nodejs >= 23.

Fix: #1089
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.

Couldn't import the type definitions of std_msgs in my Typescript program

2 participants