Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improves #57625

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Improves #57625

wants to merge 2 commits into from

Conversation

wcupped
Copy link

@wcupped wcupped commented Mar 25, 2025

No description provided.

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Mar 25, 2025
@BridgeAR
Copy link
Member

@wcupped thank you for your contribution! Could you please explain what this change intends to improve? :)

Please also briefly look into our contribution guidelines, especially the ones for pull requests: https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#the-process-of-making-changes

@wcupped
Copy link
Author

wcupped commented Mar 25, 2025

@BridgeAR

string_bytes.h Changelist:

  • Added [[nodiscard]] attributes to indicate that return values shouldn't be ignored.
  • Added noexcept specifications to improve exception safety.
  • Used std::string_view instead of raw pointers for more efficient string handling
  • Added std::optional and std::memory headers for modern C++ features
  • Replaced old C-style enum encoding with a strongly typed enum class Encoding : uint8_t. This prevents accidental conversions and provides better type safety
  • Refactored the Encode method to use std::string_view instead of raw const char*
  • Removed the overloaded Encode function with ambiguous parameters
  • Added extensive Doxygen-style documentation for the class and its methods
  • Cleared explanations of purpose, parameters, return values, and behavior
  • Added detailed comments about overestimation and performance stats
  • Added explicit delete declarations for special member functions
  • Prevents instantiation, copying, and moving of this utility class
  • Makes the intention of the class being a static utility clear
  • Adds comments about RAII-compliant memory management
  • More clearly explains the stack vs. heap buffer usage
  • Added forward declaration for the Encoding enum
  • Better separation of concerns with clear API boundaries

node_main.cc Changelist:

  • Replaced raw pointers with std::unique_ptr and std::vector for automatic memory management
  • Eliminated manual new/delete operations, reducing leak risks
  • Moved platform checks and conversions into helper functions such as CheckWindowsVersion, PrintPlatformError and ConvertWideToUTF8
  • Grouped constants and helpers in an anonymous namespace for better scoping
  • Introduced a NodeErrorCode enum for clear error categorization
  • Provided detailed error messages for conversion failures
  • Used std::make_unique for safe pointer creation
  • Leveraged std::vector to manage argument storage dynamically
  • Simplified the Windows version check logic
  • Separated concerns
  • Ensured converted arguments are properly cleaned up via RAII
  • Used argv.data() for safe array access with vectors
  • Avoided global scope pollution by encapsulating helpers in a namespace
  • Added context-specific error details (e.g., buffer size determination failures)

@wcupped
Copy link
Author

wcupped commented Mar 26, 2025

Approve check please

@jasnell
Copy link
Member

jasnell commented Mar 27, 2025

The motivation for these changes just is not clear as most appear to be largely comestic. The commits also do not follow our basic commits guidelines. This would likely be better split into multiple PRs with more of a motivation given around why these are improvements rather than just listing what was changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants