Node v22.17.1 nsolid v5.7.5 release - #341
Conversation
PR-URL: nodejs/node#58040 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
PR-URL: nodejs/node#58040 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
PR-URL: nodejs/node#58040 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
PR-URL: nodejs/node#58047 Refs: nodejs/node#47452 Refs: nodejs/node#54918 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This commit adds support for the situation where SQLite is trying to report an error while JavaScript already has an exception pending. Fixes: nodejs/node#56772 PR-URL: nodejs/node#56787 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs/node#56840 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs/node#56253 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
This commit fixes several coverity warnings related to the recently landed backup() API. PR-URL: nodejs/node#56961 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This commit updates StatementSync.prototype.run() to reset the prepared statement immediately after calling sqlite3_step() to return the correct change metadata. Fixes: nodejs/node#57344 PR-URL: nodejs/node#57350 Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
This commit adds a method for retrieving column metadata from a prepared statement. Fixes: nodejs/node#57457 PR-URL: nodejs/node#57490 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
This commit refactors the StatementSync iterator implementation in two primary ways: - The iterator internal state is no longer exposed to JavaScript. - The iterator prevents the prepared statement from being GC'ed. Fixes: nodejs/node#57493 PR-URL: nodejs/node#57569 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
PR-URL: nodejs/node#57571 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs/node#57621 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs/node#57542 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs/node#56600 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs/node#57752 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs/node#57860 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This improves the performance for almost all objects when comparing them deeply. PR-URL: nodejs/node#57648 Reviewed-By: Jordan Harband <ljharb@gmail.com> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs/node#57760 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
When passing the return value of `BackingStore::Data()` as the first or second argument to `memcpy()`, it is unnecessary to cast the returned pointer to `char*`. Refs: nodejs/node#52292 PR-URL: nodejs/node#57791 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Allow comparing a `BaseObjectPtr` or implicitly construct a `BaseObjectPtr` with `nullptr`. PR-URL: nodejs/node#56585 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: nodejs/node#57865 Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs/node#57868 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Qingyu Deng <i@ayase-lab.com> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
The latest performance optimization did not take into account that an object may have a property called constructor. This is addressed in this PR by adding a new fast path and using fallbacks. PR-URL: nodejs/node#57876 Reviewed-By: Jordan Harband <ljharb@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs/node#57852 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
PR-URL: nodejs/node#57910 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: nodejs/node#57951 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
PR-URL: nodejs/node#57943 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs/node#57969 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#57286 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
PR-URL: nodejs/node#58182 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jason Zhang <xzha4350@gmail.com>
PR-URL: nodejs/node#58258 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jordan Harband <ljharb@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
`EXTENSIONLESS_FORMAT_JAVASCRIPT` and `EXTENSIONLESS_FORMAT_WASM` are only used internally through binding `getFormatOfExtensionlessFile`. They should not be exposed publicly. PR-URL: nodejs/node#58327 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
`path.resolve()` and `path.resolve('.')` is frequently called as
alternative to process.cwd(). This minimized overhead for these
specific cases.
PR-URL: nodejs/node#58362
Reviewed-By: Jordan Harband <ljharb@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
PR-URL: nodejs/node#58350 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Fixes: nodejs/node#58402 PR-URL: nodejs/node#58403 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs/node#58400 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
For cases where the style is not needed but code still calls styleText unconditionally, this adds a `none` style that not not apply any styling to the text. PR-URL: nodejs/node#58437 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs/node#58472 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs/node#58477 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Dario Piotrowicz <dario.piotrowicz@gmail.com>
PR-URL: nodejs/node#58500 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
| Package | From | To | | --- | --- | --- | | `@babel/core` | `7.27.1` | `7.27.4` | | `@stylistic/eslint-plugin-js` | `4.2.0` | `4.4.0` | | `eslint` | `9.25.1` | `9.28.0` | | `eslint-plugin-jsdoc` | `50.6.11` | `50.7.0` | | `globals` | `16.0.0` | `16.2.0` | Co-authored-by: dependabot[bot] <support@github.com> Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: nodejs/node#58549 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
By default, the `readableWebStream` method of `FileHandle` returns a ReadableStream that, when finished, does not close the underlying FileHandle. This can lead to issues if the stream is consumed without having a reference to the FileHandle to close after use. This commit adds an `autoClose` option to the `readableWebStream` method, which, when set to `true`, will automatically close the FileHandle when the stream is finished or canceled. The test modified in this commit demonstrates one of the cases where this is necessary in that the stream is consumed by separate code than the FileHandle which was being left to close the underlying fd when it is garbage collected, which is a deprecated behavior. PR-URL: nodejs/node#58548 Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
This commit automatically includes in the allow-fs-read list all the app's entrypoints. `--require` and user entry point Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com> PR-URL: nodejs/node#58579 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
PR-URL: nodejs/node#58770 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
Notable changes: assert: * (SEMVER-MINOR) mark `partialDeepStrictEqual()` as stable (Ruben Bridgewater) nodejs/node#57370 doc: * deprecate instantiating `node:http` classes without new (Yagiz Nizipli) nodejs/node#58518 * deprecate passing an empty string to `options.shell` (Antoine du Hamel) nodejs/node#58564 * deprecate HTTP/2 priority signaling (Matteo Collina) nodejs/node#58313 * add Filip Skokan to TSC (Rafael Gonzaga) nodejs/node#58499 * add JonasBa to collaborators (Jonas Badalic) nodejs/node#58355 * add puskin to collaborators (Giovanni Bucci) nodejs/node#58308 * mark multiple APIs stable (James M Snell) nodejs/node#57513 fs: * (SEMVER-MINOR) add `autoClose` option to `FileHandle` `readableWebStream` (James M Snell) nodejs/node#58548 * (SEMVER-MINOR) add to `Dir` support for explicit resource management (Antoine du Hamel) nodejs/node#58206 http2: * (SEMVER-MINOR) add diagnostics channel `http2.server.stream.finish` (Darshan Sen) nodejs/node#58560 lib: * (SEMVER-MINOR) graduate error codes that have been around for years (James M Snell) nodejs/node#58541 src,permission: * implicit `allow-fs-read` to app entrypoint (Rafael Gonzaga) nodejs/node#58579 util: * (SEMVER-MINOR) add `'none'` style to styleText (James M Snell) nodejs/node#58437 PR-URL: nodejs/node#58665
Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com> PR-URL: nodejs-private/node-private#721 Refs: https://hackerone.com/reports/3160912 CVE-ID: CVE-2025-27210
Fixes: nodejs/node#58801 PR-URL: nodejs/node#58902 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Richard Lau <richard.lau@ibm.com>
This is a security release. Notable changes: lib: * (CVE-2025-27210) handle all windows reserved driver name PR-URL: nodejs-private/node-private#726
2025-07-15 Node.js v22.17.1 Jod (LTS) Release Git-EVTag-v0-SHA512: 8512c2241d92574e0e07873fda0b6ff07fa3a5ef51c91558cb1ce02c3d3f21c5dcc1c60d5110894c3b29ed1a9dd63398cc1fc790a12f981cf123964e394c5519
Also, make all our existing agents code to use it. Ideally having an API like this being exposed from the `node::crypto` namespace (crypto_context specifically) would be ideal, but for the time being this should do. PR-URL: #340 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
WalkthroughThis update introduces significant enhancements and maintenance across the codebase. Major changes include a comprehensive ICU upgrade to version 77 with broad API and internal refactoring, improved locale and calendar handling, new unit and measurement features, and extensive modernization of string and error handling. Additional updates address build configuration, platform support, and workflow automation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Workflow
participant ICU
participant Calendar
participant Locale
participant MeasureUnit
participant OTLPAgent
User->>Workflow: Trigger CI/Build/Benchmark
Workflow->>ICU: Build and run tests (ICU v77)
ICU->>Calendar: Compute date fields, handle leap years, error handling
ICU->>Locale: Parse and canonicalize locale IDs (string_view, normalization)
ICU->>MeasureUnit: Parse unit identifiers, handle constant denominators
ICU->>OTLPAgent: Provide root certificates for TLS
OTLPAgent->>Workflow: Secure metric export with root certs
Workflow->>User: Report results, artifacts, and coverage
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 45
🔭 Outside diff range comments (11)
.github/workflows/lint-release-proposal.yml (1)
33-35:$ACTUALis never set – causes misleading output
Both “Actual:” lines reference an undefined variable, so nothing is printed. It should echo the variable you just computed.-echo "Actual: $ACTUAL" +echo "Actual: $COMMIT_SUBJECT"Repeat the same fix later for
$PR_URL.deps/icu-small/source/i18n/olsontz.cpp (1)
478-482: Timestamp scaled 1 000× ⇒ wrong debug output
printTime(date * 1000.0)multiplies an already-millisecondUDateby 1 000,
passing micro-seconds totimeToFields(). All printed fields are therefore off
by a factor of 1 000. Drop the extra multiplication:- printTime(date*1000.0); + printTime(date);deps/icu-small/source/common/unicode/uchar.h (2)
2792-2850: *Add _COUNT sentinels for the new public enumsMost public ICU enums end with a
*_COUNT(inside#ifndef U_HIDE_DEPRECATED_API) to preserve ABI stability and to let clients determine the valid range at runtime (seeUCharDirection,UDecompositionType, etc.).
UIdentifierStatusandUIdentifierType, now promoted to stable, lack such sentinels. Adding them now avoids future source-/binary-compat issues when new values are introduced.typedef enum UIdentifierStatus { U_ID_STATUS_RESTRICTED, U_ID_STATUS_ALLOWED, +#ifndef U_HIDE_DEPRECATED_API + /** + * One more than the highest normal UIdentifierStatus value. + * @deprecated ICU 77 The numeric value may change over time. + */ + U_ID_STATUS_COUNT +#endif } UIdentifierStatus; typedef enum UIdentifierType { U_ID_TYPE_NOT_CHARACTER, ... U_ID_TYPE_RECOMMENDED, +#ifndef U_HIDE_DEPRECATED_API + /** + * One more than the highest normal UIdentifierType value. + * @deprecated ICU 77 The numeric value may change over time. + */ + U_ID_TYPE_COUNT +#endif } UIdentifierType;
4088-4105: ReturnUBool, not plain C99bool, from public C APIs
u_hasIDType()andu_getIDTypes()exposeboolin their signatures whereas the rest of the ICU C API consistently usesUBool. Mixing the types:
- breaks API uniformity,
- forces inclusion of
<stdbool.h>for C consumers (not needed elsewhere),- can lead to size/ABI mismatches on exotic platforms where
bool≠int.Consider switching to
UBool(anint8_talias) or, at minimum, adding inline/#definewrappers that forward from the oldboolversions to newUBoolvariants to avoid an ABI change.-U_CAPI bool U_EXPORT2 +U_CAPI UBool U_EXPORT2 u_hasIDType(UChar32 c, UIdentifierType type); -U_CAPI int32_t U_EXPORT2 -u_getIDTypes(UChar32 c, UIdentifierType *types, int32_t capacity, UErrorCode *pErrorCode); +U_CAPI int32_t U_EXPORT2 +u_getIDTypes(UChar32 c, UIdentifierType *types, int32_t capacity, UErrorCode *pErrorCode); /* keep signature, but each element in 'types' should now be UBool-compatible */Also applies to: 7060-7064
deps/icu-small/source/i18n/nfrule.cpp (1)
148-155: Guard the new|handling to bracket scope only
orElseOpis computed withdescription.indexOf(gVerticalLine)and later
assumed to sit between[and].
If the very first|happens to be outside that pair (or no bar is present
inside but one appears later in the rule),brack2-orElseOpor
orElseOp-brack1become negative and theUnicodeString::append
calls will read past the string or assert.int32_t orElseOp = description.indexOf(gVerticalLine); if (orElseOp >= 0 && (orElseOp <= brack1 || orElseOp >= brack2)) { // not inside [...] orElseOp = -1; // ignore – behaves like old code }This small guard keeps the fast path but prevents malformed input from
crashing the parser.Also applies to: 198-206, 213-220
deps/googletest/src/gtest-printers.cc (1)
525-551: char8_t length helper may be missing.
PrintCStringTo()relies onstd::char_traits<Char>::length().
char_traits<char8_t>is not required by the C++20 standard and is absent on libstdc++ <=13, causing a compile error for the newchar8_toverloads.Suggested fallback:
#if defined(__cpp_lib_char8_t) -template <typename Char> -static size_t SafeLength(const Char* s) { return std::char_traits<Char>::length(s); } +template <typename Char> +static size_t SafeLength(const Char* s) { return std::char_traits<Char>::length(s); } + +inline size_t SafeLength(const char8_t* s) { + const char8_t* p = s; + while (*p != u8'\0') ++p; + return static_cast<size_t>(p - s); +}and call
SafeLength()inPrintCStringTo.Without this, Ubuntu 22.04 with GCC-12 fails to build gtest.
deps/icu-small/source/i18n/unicode/calendar.h (1)
2155-2220: Bit-field compaction may hurt ABI portabilityReplacing multiple
UBool/enum members with bit-fields saves memory but:
- The in-memory layout of bit-fields is implementation-defined; mixing objects built with different compilers/flags (e.g. system vs. embedded ICU) can break.
- Using enum types (
UCalendarWallTimeOptionneeds 3 bits) assumes all values fit, yet future enum expansion will silently truncate.Consider guarding with
static_assert(sizeof(Calendar)==expected)or fall back to explicit uint8_t members to keep the external ABI stable.deps/icu-small/source/common/unicode/uset.h (1)
1656-1662: Beware danglingstd::u16string_view
operator*()builds the view from a raw pointer returned byuset_getString().
That pointer remains valid only as long as the underlyingUSetis not modified.
If client code stores the view (e.g. in a container) and the set is later mutated or destroyed, the view becomes dangling.Document the lifetime contract clearly or return a
std::u16stringto avoid accidental misuse.deps/icu-small/source/i18n/messageformat2.cpp (1)
44-50: Null-pointer dereference when global argument is missing
context.getGlobal()can legally returnnullptr(unbound var with no error).FormattedPlaceholder(*val, …)immediately dereferences the pointer without checking, risking a crash.
Add a nullptr check and fall back to the$varplaceholder before dereferencing.deps/icu-small/source/common/uloc.cpp (2)
1147-1154: Duplicate definition of_hasBCP47Extension– will not compileFunction
_hasBCP47Extension(std::string_view)is already defined earlier (lines 510-512) with a different implementation.
Two inline definitions with the same signature in the same translation unit violate the One-Definition Rule and break the build.-inline bool _hasBCP47Extension(std::string_view p) { - return p.size() >= 3 && - p[0] == '-' && - (p[1] == 't' || p[1] == 'T' || - p[1] == 'u' || p[1] == 'U' || - p[1] == 'x' || p[1] == 'X') && - p[2] == '-'; -} +// ‼ remove this second definition OR merge the two into a single, shared implementation
525-546: Ensurelocale_getKeywordsStartreturns a null-terminated C‐string or astd::string_view
- The function currently does
butreturn localeID.data() + pos;std::string_viewdoes not guarantee thatdata()[size()] == '\0'. Downstream calls to C‐string APIs (e.g.strchr,uprv_strcmp) can overread past the view’s end.- Two safe options:
- Change the API to return
std::string_viewinstead ofconst char*, so callers handle substring lengths without relying on a NUL‐terminator.- If C‐string semantics must be preserved, allocate or ensure a trailing
'\0'after the returned pointer (e.g. copy the suffix into a small buffer and NUL‐terminate).Affected location:
deps/icu-small/source/common/uloc.cpp, functionlocale_getKeywordsStart(lines 525–546)
♻️ Duplicate comments (1)
deps/icu-small/source/i18n/dtfmtsym.cpp (1)
540-544: Duplicate deletion handled above.These lines are the correct place to own and clear the two pointers; keep them and remove the destructor duplicates noted earlier.
🧹 Nitpick comments (40)
deps/icu-small/source/i18n/unicode/ucol.h (1)
1543-1616: Excellent modernization to standard C++ string views.The refactoring from ICU-specific string types to standard C++ string views improves code maintainability and standard library compatibility. This aligns well with modern C++ practices.
Consider adding a comment explaining the rationale for supporting multiple string view types to help future maintainers understand the design decisions.
.github/workflows/lint-release-proposal.yml (1)
23-25: Redundant-but-harmless permission duplication
You already grantcontents: readat the workflow level. Re-declaring it at the job level is fine but unnecessary. If the intent is job-scoped tightening, consider dropping the top-level block to avoid confusion..github/workflows/daily-wpt-fyi.yml (1)
52-57: Double-checkactions/setup-nodepin.Same concern for
49933ea5288caeca8642d1e84afbd3f7d6820020vs. the v4.4.0 tag; run the analogousgh apicheck.While here, consider using Dependabot to keep these pins current automatically.
deps/icu-small/source/i18n/messageformat2_errors.cpp (1)
6-7: Flatten the top-level feature guard to reduce nested#ifspaghettiThe entire compilation unit now sits behind
!UCONFIG_NO_NORMALIZATION, plus two nested guards for!UCONFIG_NO_FORMATTINGand!UCONFIG_NO_MF2.
Consider moving the new normalization check to the public header that declares the APIs or collapsing the three guards into a single composite condition:#if !UCONFIG_NO_FORMATTING && !UCONFIG_NO_MF2 && !UCONFIG_NO_NORMALIZATIONThis keeps the file body un-indented, makes the closing
#endifunambiguous, and avoids future mismatches as more code is added.Also applies to: 295-296
.github/workflows/coverage-linux-without-intl.yml (1)
58-59: Use version tags unless you need the exact commitThe actions are now pinned by full commit SHA.
If deterministic builds are not a requirement here, switching back to the semantic tag (actions/setup-python@v5.6.0,Mozilla-Actions/sccache-action@v0.0.9,codecov/codecov-action@v5.4.3) makes future upgrades easier while still being immutable.Also applies to: 62-63, 81-82
deps/icu-small/source/common/locdispnames.cpp (1)
382-385: Graceful fallback added – consider also guarding empty stringsGood call replacing a null
localewithuloc_getDefault().
If the API may also receive an empty string (length == 0), you might want to treat it the same way to avoid passing""down-stream, e.g.if (locale == nullptr || *locale == '\0') { locale = uloc_getDefault(); }.github/workflows/codeql.yml (1)
3-6: Trigger runs only on nightly cronAt the moment the scan will not run for push / pull-request events, so new code could reach
mainun-scanned for ≤ 24 h.
If that gap is undesired, add:on: push: branches: [ main ] pull_request: branches: [ main ] schedule: - cron: '0 0 * * *'deps/icu-small/source/i18n/indiancal.h (1)
206-213: Doxygen comment is now stale
handleGetYearLengthgained aUErrorCode& statusparameter, but the doc-block and@paramlist were not updated (it also listsyeartwice).
Updating comments avoids confusion for downstream users.deps/icu-small/source/common/loclikelysubtags.cpp (1)
528-535: Update in-line comment to match new conditionEarly-return for
@x=private-use tags now happens only whenreturnInputIfUnmatchis false, but the explanatory comment didn’t change.
Tweaking the comment keeps the intent crystal-clear..github/workflows/tools.yml (1)
287-294: Label looks misleading for the newzstdjob.The matrix entry inherits
label: dependencies, zlib, yet the dependency being updated iszstd.
Unlesszstdpurposefully shares the same “zlib” label for downstream automation, this will file PRs under an incorrect label and may confuse reviewers/triagers.- label: dependencies, zlib + label: dependencies, zstdIf “zlib” is indeed required by tooling, add a clarifying comment.
benchmark/sqlite/sqlite-prepare-select-all.js (1)
1-1: Remove redundant 'use strict' directive.In ES modules and modern Node.js, the 'use strict' directive is redundant as modules are automatically in strict mode.
-'use strict'; const common = require('../common.js');benchmark/sqlite/sqlite-prepare-insert.js (2)
1-1: Remove redundant 'use strict' directive.In ES modules and modern Node.js, the 'use strict' directive is redundant as modules are automatically in strict mode.
-'use strict'; const common = require('../common.js');
44-97: Consider refactoring control flow for better maintainability.The current approach using string matching in if-else statements works but could be improved for maintainability and readability.
Consider using a mapping approach:
- if (conf.statement.includes('text_column_type')) { - // ... benchmark logic - } else if (conf.statement.includes('integer_column_type')) { - // ... benchmark logic - } // ... more else-if blocks + const benchmarkHandlers = { + 'text_column_type': () => insertStatement.run(stringValue), + 'integer_column_type': () => insertStatement.run(integerValue), + 'real_column_type': () => insertStatement.run(realValue), + 'blob_column_type': () => insertStatement.run(blobValue), + 'INTO all_column_types': () => insertStatement.run(stringValue, integerValue, realValue, blobValue), + 'INTO large_text': () => insertStatement.run(largeText), + 'missing_required_value': () => { + try { + return insertStatement.run(stringValue); + } catch (e) { + return e; + } + } + }; + + const handler = Object.entries(benchmarkHandlers).find(([key]) => + conf.statement.includes(key) + )?.[1]; + + if (!handler) { + throw new Error('Unknown statement'); + } + + bench.start(); + for (i = 0; i < conf.n; i += 1) { + deadCodeElimination = handler(); + } + bench.end(conf.n);deps/googletest/src/gtest.cc (1)
1490-1498: Header comment is now misleading – update to match new conditional logicThe doc-comment says
@@ -<left_start>,<left_length> +<right_start>,<right_length> @@which implies that both
<left_length>and<right_length>are always present.
The actual code still omits the “,<len>” part when the length equals 1:if (left_length != 1) *ss << "," << left_length; ... if (right_length != 1) *ss << "," << right_length;That behaviour (omit when length == 1) is fine and consistent with GNU diff, but the
comment should explicitly say so to avoid confusion for future maintainers.-// "@@ -<left_start>,<left_length> +<right_start>,<right_length> @@" +// "@@ -<left_start>[,<left_length>] +<right_start>[,<right_length>] @@" +// Length is omitted when it equals 1, matching GNU diff’s convention.deps/icu-small/source/i18n/olsontz.cpp (1)
437-446: Error status is never inspected
Grego::timeToFields()populatesstatus, but the debug helper discards the value,
so corrupt values could be logged while masking an internal failure.
Even in debug-only code, checkU_SUCCESS(status)(or reset it before the call) to
avoid accidental misuse in future refactors.common.gypi (2)
509-515: Update OS lists in a single place
openharmonywas added to several OS condition lists, but not all (e.g. the
large block starting at line 581). Keeping the lists in sync prevents silent
feature drift.
466-468: Duplicate manual define
V8_EXTERNAL_CODE_SPACEis injected here, yet v8 already emits the define
internally when the flag is on. Redundant defines are harmless but noisy; feel
free to drop this stanza.deps/cares/config/openharmony/ares_config.h (1)
7-9: Typo in comment
non-pubilc→non-public. Harmless but easy to fix while the file is new.benchmark/sqlite/sqlite-prepare-select-get.js (1)
1-1: Remove redundant "use strict" directive.The "use strict" directive is redundant in JavaScript modules as they are automatically in strict mode.
-'use strict';deps/icu-small/source/i18n/collationruleparser.cpp (1)
622-634: Consider simplifying the "und" prepending logicWhile the logic for handling baseIDs starting with '_' is correct, the implementation using
getAppendBufferand manual memory operations seems overly complex for prepending a simple string.Consider using a simpler approach:
- } else if (baseID[0] == '_') { - // CharString doesn't have any insert() method, only append(). - constexpr char und[] = "und"; - constexpr int32_t length = sizeof und - 1; - int32_t dummy; - char* tail = baseID.getAppendBuffer(length, length, dummy, errorCode); - char* head = baseID.data(); - uprv_memmove(head + length, head, baseID.length()); - uprv_memcpy(head, und, length); - baseID.append(tail, length, errorCode); + } else if (baseID[0] == '_') { + // Prepend "und" to baseID starting with '_' + CharString temp("und", errorCode); + temp.append(baseID, errorCode); + baseID = std::move(temp);This approach is more readable and less error-prone.
benchmark/assert/deepequal-set.js (1)
20-24: Clarify the combinationFilter logicThe filter condition
p.order !== 'random' || p.strict === 1 && p.method !== 'notDeepEqual_objectOnly'is complex and could benefit from clarification.Consider adding a comment to explain the filtering logic:
combinationFilter(p) { + // Allow all non-random orders, or for random order: only allow when not strict mode + // or when the method is 'notDeepEqual_objectOnly' return p.order !== 'random' || p.strict === 1 && p.method !== 'notDeepEqual_objectOnly'; },deps/icu-small/source/common/unicode/uniset.h (1)
1170-1182: Collapse duplicate draft-API guards for clarityThe file now closes
#ifndef U_HIDE_DRAFT_APIat line 1176 and immediately
opens a new one two lines later. This compiles but obscures intent and
creates unnecessary diff noise. Keeping all draft APIs inside a single
guard improves readability.-#endif // U_HIDE_DRAFT_API - -#ifndef U_HIDE_DRAFT_API +// (continues inside the same draft-API guard)Also applies to: 1176-1180
deps/icu-small/source/common/locid.cpp (1)
1837-1990: Good use of modern C++ patterns, but consider breaking up this methodThe lambda at lines 1865-1879 nicely encapsulates the canonicalization logic using ByteSink. However, this 150+ line method could benefit from being broken into smaller, more focused helper methods for better readability and maintainability.
deps/icu-small/source/i18n/messageformat2_checker.cpp (1)
200-200: Consider caching normalized keys for performanceThe normalization is performed repeatedly in nested loops (O(n²) comparisons). Consider normalizing all keys once and caching the results to improve performance for messages with many variants.
deps/icu-small/source/i18n/number_longnames.cpp (2)
1208-1212: Consider adding error checking after number conversion.While the status parameter is passed to
appendNumber, consider explicitly checking for errors before proceeding:if (productUnit.constantDenominator != 0) { CharString constantString; constantString.appendNumber(productUnit.constantDenominator, status); + if (U_FAILURE(status)) { return; } outArray[CONSTANT_DENOMINATOR_INDEX] = UnicodeString::fromUTF8(constantString.toStringPiece()); }
1472-1504: Implementation looks correct, but note the TODOs for future improvements.The constant denominator handling logic is well-structured. The TODO comments at lines 1485-1494 correctly identify areas for future enhancement:
- Better handling of large constant denominators (e.g., formatting 1000000 as "1 million")
- Consistent pluralization rules for units with constant denominators
Consider creating tracking issues for these improvements to ensure they're not forgotten.
Would you like me to help create GitHub issues to track these TODO items?
deps/icu-small/source/common/locbased.cpp (1)
51-66: Possible redundant allocation & copy
setLocaleID(const char* …)always allocates a freshCharStringeven whendestalready contains an identical value.
A quick equality check before allocating/copying would avoid needless heap churn in hot paths (e.g.,LocaleBasedsetters executed by every formatter):- if (dest == nullptr) { + if (dest == nullptr || dest->compare(id) != 0) {(Do the same in the
CharString*overload.)
Not critical, but improves perf and reduces GC pressure.deps/icu-small/source/i18n/persncal.cpp (2)
153-164: Initialisation order nit – readgMinCorrectiononly afterinitOnce
isLeapYear()readsgMinCorrectionbefore the one-time initialiser runs.
On the first invocation this value is still zero, so the first comparison is effectivelyyear >= 0, which is harmless but misleading and adds an unnecessaryUnicodeSet::containscall.Moving the
getLeapCorrection()call (which triggersinitOnce) to the top makes the logic clearer and future-proof:const auto* corr = getLeapCorrection(); // ensures gMinCorrection is set if (year >= gMinCorrection && corr->contains(year)) return false; if (year > gMinCorrection && corr->contains(year-1)) return true;Purely a readability / micro-perf tweak.
217-223: Minor: duplicate leap-year correction infirstJulianOfYear
firstJulianOfYear()again checks(year > gMinCorrection && corr->contains(year-1))which is already handled inisLeapYear().
Consider keeping leap logic in one place (e.g., viaisLeapYear) to avoid divergence.Not blocking, just maintainability advice.
deps/icu-small/source/common/uloc_tag.cpp (1)
2648-2654: Minor perf nit – avoid repeated CharString conversions
_append*ToLanguageTag()are called sequentially with the samecanonicalvalue, each time re-deriving language/region/etc.
Caching these pieces once (e.g. a struct with the split fields) would avoid several extraCharStringallocations per call.
Not urgent, but easy head-room for hot paths like locale conversion.deps/icu-small/source/i18n/gregoimp.cpp (3)
120-144: Potential narrowing when writing toint8_toutputs
month,dom, anddoware nowint8_t.
The computed values are within 0-11 / 1-31 / 1-7, so they fit, but the intermediate arithmetic usesint32_t; an explicit cast would silence compiler truncation warnings and document intent:- month = (12 * (doy - 1 + correction) + 6) / 367; - dom = doy - DAYS_BEFORE[month + (isLeap ? 12 : 0)]; + month = static_cast<int8_t>((12 * (doy - 1 + correction) + 6) / 367); + dom = static_cast<int8_t>(doy - DAYS_BEFORE[month + (isLeap ? 12 : 0)]);
191-202: Chained overloads add maintenance overheadThe three new
timeToFieldsoverloads differ only by unused out-params.
Consider delegating to a single core helper with defaults to avoid the current pyramid of wrappers.
206-222: Implicit double→int32 narrowing
dayisdouble, butdayToFields/dayToYeartakeint32_t; the implicit cast is safe after the explicit range check, yet an explicitstatic_cast<int32_t>(day)would improve readability and silence static-analysis warnings.deps/icu-small/source/i18n/gregoimp.h (1)
217-228: Consider adding[[nodiscard]]attribute to conversion methodsThe static methods that perform conversions should have the
[[nodiscard]]attribute (if C++17 is available) to prevent accidental misuse where the error code is checked but the result is ignored.For the methods returning values (like
dayToYearandtimeToYear), consider:- static int32_t dayToYear(int32_t day, UErrorCode& status); + [[nodiscard]] static int32_t dayToYear(int32_t day, UErrorCode& status);Also applies to: 230-241, 243-256, 259-272, 274-286, 289-299, 301-306
deps/icu-small/source/i18n/unicode/messageformat2.h (1)
455-457: Member not initialized by default constructors
nfcNormalizeris default-initialised to nullptr here but only set in
some constructors. Ensure every constructor (including move-constructor)
assigns it viaNormalizer2::getNFCInstance(status).deps/icu-small/source/common/unicode/uset.h (1)
1768-1778: Heavy allocation on every dereference
operator*()constructs a freshstd::u16stringfor each code-point or string element, causing repeated heap allocations during iteration.If callers mostly read the element once, return
std::u16string_view(likeUSetStringIterator) or reuse an internal buffer to amortise allocations.deps/icu-small/source/i18n/messageformat2_parser.h (1)
158-166: Duplicate “required-whitespace” APIsBoth
parseRequiredWS()and the legacyparseRequiredWhitespace()now exist.
Maintaining two names for the same concept increases the risk of accidental divergence.Pick one, deprecate/remove the other, and update call-sites for consistency.
deps/icu-small/source/i18n/measunit_extra.cpp (1)
1293-1315: Duplicated “per” prefix risk
getConstantsString()returns only the numeric part, but the header comment says it yields “-per-1000”.
If future maintainers rely on that comment they might prepend an extra “per”, breaking identifiers.Synchronise the comment with the code and add unit tests to catch regressions.
deps/icu-small/source/i18n/calendar.cpp (2)
1156-1159: Nit: drop the manual loop – but useunsigned charcast explicitlyUsing
uprv_memset(fStamp, kUnset, sizeof(fStamp));is fine now that
fStampis 8-bit, yet it relies on implementation-defined sign/char
conversion. A tiny cast avoids surprises:- uprv_memset(fStamp, kUnset, sizeof(fStamp)); + uprv_memset(fStamp, static_cast<unsigned char>(kUnset), sizeof(fStamp));
4222-4241: Compression loop resetsfNextStamptokInternallySetAfter the first iteration
fNextStamp==kInternallySet + 1; if
kInternallySetever equalsSTAMP_MAXthe loop will spin forever.
Worth adding a sanity assert thatkInternallySet < STAMP_MAX.
| bool match(std::u16string_view lhs, std::u16string_view rhs) const { | ||
| return compare( | ||
| ucol_strcoll( | ||
| collator, | ||
| toUCharPtr(lhs.getBuffer()), lhs.length(), | ||
| toUCharPtr(rhs.getBuffer()), rhs.length()), | ||
| toUCharPtr(lhs.data()), static_cast<int32_t>(lhs.length()), | ||
| toUCharPtr(rhs.data()), static_cast<int32_t>(rhs.length())), | ||
| result); | ||
| } | ||
|
|
||
| bool match(StringPiece lhs, StringPiece rhs) const { | ||
| bool match(std::string_view lhs, std::string_view rhs) const { | ||
| UErrorCode status = U_ZERO_ERROR; | ||
| return compare( | ||
| ucol_strcollUTF8( | ||
| collator, | ||
| lhs.data(), lhs.length(), | ||
| rhs.data(), rhs.length(), | ||
| lhs.data(), static_cast<int32_t>(lhs.length()), | ||
| rhs.data(), static_cast<int32_t>(rhs.length()), | ||
| &status), | ||
| result); | ||
| } |
There was a problem hiding this comment.
Add safety checks for string length conversions.
The static_cast from size_t to int32_t could overflow for very large strings. Consider adding checks or using safe conversion utilities.
Also, the error status in the UTF-8 match method should be checked after the ucol_strcollUTF8 call.
bool match(std::u16string_view lhs, std::u16string_view rhs) const {
+ if (lhs.length() > INT32_MAX || rhs.length() > INT32_MAX) {
+ // Handle overflow case
+ return false;
+ }
return compare(
ucol_strcoll(
collator,
toUCharPtr(lhs.data()), static_cast<int32_t>(lhs.length()),
toUCharPtr(rhs.data()), static_cast<int32_t>(rhs.length())),
result);
}
bool match(std::string_view lhs, std::string_view rhs) const {
+ if (lhs.length() > INT32_MAX || rhs.length() > INT32_MAX) {
+ // Handle overflow case
+ return false;
+ }
UErrorCode status = U_ZERO_ERROR;
- return compare(
+ UCollationResult coll_result =
ucol_strcollUTF8(
collator,
lhs.data(), static_cast<int32_t>(lhs.length()),
rhs.data(), static_cast<int32_t>(rhs.length()),
- &status),
- result);
+ &status);
+ if (U_FAILURE(status)) {
+ // Handle error appropriately
+ return false;
+ }
+ return compare(coll_result, result);
}📝 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.
| bool match(std::u16string_view lhs, std::u16string_view rhs) const { | |
| return compare( | |
| ucol_strcoll( | |
| collator, | |
| toUCharPtr(lhs.getBuffer()), lhs.length(), | |
| toUCharPtr(rhs.getBuffer()), rhs.length()), | |
| toUCharPtr(lhs.data()), static_cast<int32_t>(lhs.length()), | |
| toUCharPtr(rhs.data()), static_cast<int32_t>(rhs.length())), | |
| result); | |
| } | |
| bool match(StringPiece lhs, StringPiece rhs) const { | |
| bool match(std::string_view lhs, std::string_view rhs) const { | |
| UErrorCode status = U_ZERO_ERROR; | |
| return compare( | |
| ucol_strcollUTF8( | |
| collator, | |
| lhs.data(), lhs.length(), | |
| rhs.data(), rhs.length(), | |
| lhs.data(), static_cast<int32_t>(lhs.length()), | |
| rhs.data(), static_cast<int32_t>(rhs.length()), | |
| &status), | |
| result); | |
| } | |
| bool match(std::u16string_view lhs, std::u16string_view rhs) const { | |
| if (lhs.length() > INT32_MAX || rhs.length() > INT32_MAX) { | |
| // Handle overflow case | |
| return false; | |
| } | |
| return compare( | |
| ucol_strcoll( | |
| collator, | |
| toUCharPtr(lhs.data()), static_cast<int32_t>(lhs.length()), | |
| toUCharPtr(rhs.data()), static_cast<int32_t>(rhs.length())), | |
| result); | |
| } | |
| bool match(std::string_view lhs, std::string_view rhs) const { | |
| if (lhs.length() > INT32_MAX || rhs.length() > INT32_MAX) { | |
| // Handle overflow case | |
| return false; | |
| } | |
| UErrorCode status = U_ZERO_ERROR; | |
| UCollationResult coll_result = | |
| ucol_strcollUTF8( | |
| collator, | |
| lhs.data(), static_cast<int32_t>(lhs.length()), | |
| rhs.data(), static_cast<int32_t>(rhs.length()), | |
| &status); | |
| if (U_FAILURE(status)) { | |
| // Handle error appropriately | |
| return false; | |
| } | |
| return compare(coll_result, result); | |
| } |
🤖 Prompt for AI Agents
In deps/icu-small/source/i18n/unicode/ucol.h around lines 1594 to 1612, the
static_cast from size_t to int32_t for string lengths can overflow with very
large strings, and the UErrorCode status from ucol_strcollUTF8 is not checked.
Fix this by adding checks to ensure the string length fits within int32_t before
casting, and after calling ucol_strcollUTF8, verify the status for errors and
handle them appropriately.
| # Install dependencies | ||
| - name: Install Node.js | ||
| uses: actions/setup-node@1d0ff469b7ec7b3cb9d8673fde0c81c44821de2a # v4.2.0 | ||
| uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Version bump LGTM – double-check the pin across the repo.
The SHA now points to actions/setup-node@v4.4.0. Looks good and keeps the reproducible-build guarantee. To avoid drift, please confirm that every other workflow has been bumped to the same SHA (it looks so from this PR, but a quick grep never hurts).
🏁 Script executed:
#!/bin/bash
# Verify that only the expected SHA is referenced for setup-node.
rg --no-heading --line-number $'uses: actions/setup-node@' -g '*.yml' | grep -v '49933ea5288caeca8642d1e84afbd3f7d6820020' || trueLength of output: 355
Update all workflow pins to the new setup-node SHA
The linters workflow is still on v4.2.0; please bump these to the new SHA to keep consistency and reproducible builds:
• File: .github/workflows/linters.yml
Lines 32, 62
- uses: actions/setup-node@1d0ff469b7ec7b3cb9d8673fde0c81c44821de2a # v4.2.0
+ uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .github/workflows/linters.yml at lines 32 and 62, update the setup-node
action version to use the new SHA 49933ea5288caeca8642d1e84afbd3f7d6820020,
matching the commit-queue.yml file at line 75. This ensures consistency and
reproducible builds across workflows by pinning to the same action version.
| cnv->toULength = static_cast<int8_t>(source - lastSource); | ||
| do { | ||
| *bytes++=*lastSource++; | ||
| } while(lastSource<source); | ||
| uprv_memcpy(cnv->toUBytes, lastSource, cnv->toULength); | ||
| *pErrorCode=U_TRUNCATED_CHAR_FOUND; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential overflow: guard uprv_memcpy() with a size check
cnv->toULength can carry any value returned from the UTF-8 length logic (up to 6 for exotic/invalid sequences).
toUBytes is fixed‐size (4 bytes). An invalid 5- or 6-byte starter would overrun the buffer.
cnv->toULength = static_cast<int8_t>(source - lastSource);
- uprv_memcpy(cnv->toUBytes, lastSource, cnv->toULength);
+ U_ASSERT(cnv->toULength <= static_cast<int8_t>(sizeof(cnv->toUBytes)));
+ if (cnv->toULength > static_cast<int8_t>(sizeof(cnv->toUBytes))) {
+ cnv->toULength = sizeof(cnv->toUBytes); /* truncate defensively */
+ }
+ uprv_memcpy(cnv->toUBytes, lastSource, cnv->toULength);A cheap U_ASSERT (or runtime clamp in release builds) prevents a silent heap smash while preserving the new, cleaner copy.
🤖 Prompt for AI Agents
In deps/icu-small/source/common/ucnvmbcs.cpp around lines 3149 to 3151, the call
to uprv_memcpy uses cnv->toULength as the copy size without checking if it
exceeds the fixed buffer size of toUBytes (4 bytes), risking buffer overflow.
Add a check to ensure cnv->toULength does not exceed 4 before the memcpy call,
using an assertion or clamp the length to 4 in release builds to prevent
overruns while preserving functionality.
| // -Wreturn-local-addr first found in https://gcc.gnu.org/onlinedocs/gcc-4.8.5/gcc/Warning-Options.html#Warning-Options | ||
| #if U_GCC_MAJOR_MINOR >= 409 | ||
| #pragma GCC diagnostic push | ||
| #pragma GCC diagnostic ignored "-Wreturn-local-addr" | ||
| #endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard the pragma with an explicit compiler check
Unknown pragmas are tolerated by most compilers but can still show up as warnings (MSVC, ICC, some embedded tool-chains).
Wrapping the diagnostic push/pop with an __GNUC__ check keeps the intent while eliminating spurious “unknown pragma” noise outside GCC/Clang.
-#if U_GCC_MAJOR_MINOR >= 409
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wreturn-local-addr"
-#endif
+#if defined(__GNUC__) && U_GCC_MAJOR_MINOR >= 409
+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Wreturn-local-addr"
+#endif
…
-#if U_GCC_MAJOR_MINOR >= 409
-#pragma GCC diagnostic pop
-#endif
+#if defined(__GNUC__) && U_GCC_MAJOR_MINOR >= 409
+# pragma GCC diagnostic pop
+#endifNo behavioural change; purely build-hygiene.
Also applies to: 221-223
🤖 Prompt for AI Agents
In deps/icu-small/source/i18n/formattedvalue.cpp around lines 196 to 200 and
also lines 221 to 223, the GCC diagnostic pragmas are not explicitly guarded by
a compiler check, causing warnings on non-GCC compilers. Wrap the #pragma GCC
diagnostic push/pop and ignored directives inside an #ifdef __GNUC__ block to
ensure they are only applied when compiling with GCC or compatible compilers,
preventing unknown pragma warnings on other toolchains.
| # Disabled because "Report JS" step was crashing. | ||
| if: false | ||
| runs-on: ubuntu-24.04 |
There was a problem hiding this comment.
Coverage job is now permanently disabled
if: false stops the entire coverage-linux job.
Unless this is a short-term workaround, it removes our only Linux coverage gate and will cause Codecov to show 0 % for future commits.
- # Disabled because "Report JS" step was crashing.
- if: false
+ # TODO: re-enable once "Report JS" crash is fixed.
+ if: github.event.pull_request.draft == falsePlease confirm this is intentional or re-enable behind a feature flag.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .github/workflows/coverage-linux.yml at lines 50 to 52, the coverage-linux
job is disabled by setting `if: false`, which stops the entire job and removes
Linux coverage reporting. To fix this, either remove or modify the `if: false`
condition to re-enable the job, ideally gating it behind a feature flag or
conditional that allows it to run when appropriate, ensuring Linux coverage is
reported and Codecov does not show 0% for future commits.
| uint64_t otherConstantDenominator = other.getConstantDenominator(status); | ||
|
|
||
| // TODO: we can also multiply the constant denominators instead of returning an error. | ||
| if (currentConstatDenominator != 0 && otherConstantDenominator != 0) { | ||
| // There is only `one` constant denominator in a compound unit. | ||
| // Therefore, we Cannot multiply units that both of them have a constant denominator | ||
| status = U_ILLEGAL_ARGUMENT_ERROR; | ||
| return {}; | ||
| } | ||
|
|
||
| // Because either one of the constant denominators is zero, we can use the maximum of them. | ||
| impl.constantDenominator = uprv_max(currentConstatDenominator, otherConstantDenominator); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ambiguous rule when multiplying units each with a denominator
MeasureUnit::product() forbids both operands carrying a constant denominator, returning U_ILLEGAL_ARGUMENT_ERROR.
This is surprising – multiplying “m-per-1000” by “s-per-60” is mathematically well-defined.
Consider computing the LCM or product of denominators, or at least document the restriction prominently.
🤖 Prompt for AI Agents
In deps/icu-small/source/i18n/measunit_extra.cpp around lines 1502 to 1514, the
current code forbids multiplying two units if both have a constant denominator,
returning an error. To fix this, replace the error return with logic that
computes the least common multiple (LCM) or product of the two denominators to
combine them correctly. Update the constantDenominator assignment to use this
combined denominator instead of just taking the maximum. Alternatively, if this
is not feasible now, add clear documentation explaining this restriction
prominently.
| static uint64_t parseStringToLong(const StringPiece strNum, UErrorCode &status) { | ||
| // We are processing well-formed input, so we don't need any special options to | ||
| // StringToDoubleConverter. | ||
| StringToDoubleConverter converter(0, 0, 0, "", ""); | ||
| int32_t count; | ||
| double double_result = converter.StringToDouble(strNum.data(), strNum.length(), &count); | ||
| if (count != strNum.length()) { | ||
| status = kUnitIdentifierSyntaxError; | ||
| return 0; | ||
| } | ||
|
|
||
| if (U_FAILURE(status) || double_result < 1.0 || double_result > static_cast<double>(INT64_MAX)) { | ||
| status = kUnitIdentifierSyntaxError; | ||
| return 0; | ||
| } | ||
|
|
||
| // Check if the value is integer. | ||
| uint64_t int_result = static_cast<uint64_t>(double_result); | ||
| const double kTolerance = 1e-9; | ||
| if (abs(double_result - int_result) > kTolerance) { | ||
| status = kUnitIdentifierSyntaxError; | ||
| return 0; | ||
| } | ||
|
|
||
| return int_result; | ||
| } |
There was a problem hiding this comment.
Integer parsing via double loses precision for large constants
parseStringToLong() converts the text to double and back.
All integers above 2⁵³ (~9 × 10¹⁵) cannot be represented exactly in IEEE-754, so many valid 64-bit constants will spuriously fail the “is integer” test.
Parse directly with std::strtoull/std::from_chars (C++17) or a manual loop to handle the optional exponent without precision loss.
🤖 Prompt for AI Agents
In deps/icu-small/source/i18n/measunit_extra.cpp around lines 558 to 583, the
parseStringToLong function currently converts the input string to double and
back, causing precision loss for large integers above 2^53. To fix this, replace
the double conversion with a direct integer parsing method such as std::strtoull
or std::from_chars (if C++17 is available), or implement a manual digit-by-digit
parsing loop that correctly handles optional exponents without precision loss.
This will ensure accurate parsing of 64-bit integer values.
| UErrorCode status = U_ZERO_ERROR; | ||
| U_LOCALE_BASED(locBased, *this); | ||
| locBased.setLocaleIDs(right.validLocale, right.actualLocale, status); | ||
| U_ASSERT(U_SUCCESS(status)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assignment operator swallows allocation failures in release builds
locBased.setLocaleIDs(...) sets status, but the result is only U_ASSERT-ed.
In non-debug builds the failure is silently ignored, leaving validLocale/actualLocale
dangling. Propagate the error (or throw) instead of relying on the assert.
🤖 Prompt for AI Agents
In deps/icu-small/source/i18n/calendar.cpp around lines 817 to 821, the
assignment operator calls locBased.setLocaleIDs which sets a status code, but
only asserts success, causing allocation failures to be ignored in release
builds. Modify the code to check the status after setLocaleIDs and propagate the
error or throw an exception if it indicates failure, instead of relying solely
on U_ASSERT, to prevent leaving validLocale and actualLocale in an invalid
state.
| if (fNextStamp == STAMP_MAX) { | ||
| recalculateStamp(); | ||
| } | ||
| fStamp[field] = fNextStamp++; | ||
| fIsSet[field] = true; // Remove later | ||
| fIsTimeSet = fAreFieldsSet = fAreFieldsVirtuallySet = false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Off-by-one guard – use >= STAMP_MAX instead of ==
If fNextStamp got corrupted and already exceeded STAMP_MAX,
the current equality check would miss it and overflow the
8-bit fStamp. A defensive comparison is safer:
- if (fNextStamp == STAMP_MAX) {
+ if (fNextStamp >= STAMP_MAX) {🤖 Prompt for AI Agents
In deps/icu-small/source/i18n/calendar.cpp around lines 1194 to 1199, replace
the equality check 'fNextStamp == STAMP_MAX' with a greater-than-or-equal check
'fNextStamp >= STAMP_MAX' to prevent overflow if fNextStamp exceeds STAMP_MAX,
ensuring the guard condition is more robust and defensive.
| { | ||
| return handleComputeMonthStart(extendedYear, month+1, true, status) - | ||
| int32_t nextMonth; | ||
| if (uprv_add32_overflow(month, 1, &nextMonth)) { | ||
| status = U_ILLEGAL_ARGUMENT_ERROR; | ||
| return 0; | ||
| } | ||
| return handleComputeMonthStart(extendedYear, nextMonth, true, status) - | ||
| handleComputeMonthStart(extendedYear, month, true, status); |
There was a problem hiding this comment.
Possible month overflow in handleGetMonthLength
nextMonth = month + 1 passes 12 for December.
handleComputeMonthStart() traditionally expects 0-11; relying on
its internal wrap-around is brittle and undefined for some calendar
sub-classes. Safer:
-int32_t nextMonth = month + 1;
-return handleComputeMonthStart(extendedYear, nextMonth, true, status) -
+int32_t nextMonth = (month + 1) % 12;
+int32_t nextYear = extendedYear + ((month == 11) ? 1 : 0);
+return handleComputeMonthStart(nextYear, nextMonth, true, status) -
handleComputeMonthStart(extendedYear, month, true, status);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In deps/icu-small/source/i18n/calendar.cpp around lines 3808 to 3815, the code
increments the month by one and passes it directly to handleComputeMonthStart,
which expects months in the range 0-11. To fix this, adjust nextMonth to wrap
around to 0 when month is 11 (December) before calling handleComputeMonthStart,
ensuring the month value stays within the valid range and avoiding reliance on
internal wrap-around behavior.
| const Operand& rand = expr.getOperand(); | ||
| // Format the operand (formatOperand handles the case of a null operand) | ||
| FormattedPlaceholder randVal = formatOperand(globalEnv, rand, context, status); | ||
| LocalPointer<InternalValue> randVal(formatOperand(globalEnv, rand, context, status)); | ||
|
|
||
| // Don't call the function on error values | ||
| if (randVal.isFallback()) { | ||
| return randVal; | ||
| } | ||
| FormattedPlaceholder maybeRand = randVal->takeArgument(status); | ||
|
|
||
| if (!expr.isFunctionCall()) { | ||
| if (!expr.isFunctionCall() && U_SUCCESS(status)) { | ||
| // Dispatch based on type of `randVal` | ||
| return evalFormatterCall(std::move(randVal), | ||
| context, | ||
| status); | ||
| } else { | ||
| if (maybeRand.isFallback()) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unchecked allocation path leads to seg-faults
randVal comes from create<InternalValue>(); on OOM it is nullptr, yet it is dereferenced on the very next line. Similar unchecked paths exist throughout this file (partVal, rv, etc.).
Guard every dereference or early-return on randVal == nullptr.
🤖 Prompt for AI Agents
In deps/icu-small/source/i18n/messageformat2.cpp around lines 238 to 246, the
pointer randVal returned by formatOperand can be nullptr on out-of-memory
conditions but is dereferenced immediately without a null check, risking
segmentation faults. Add a check after allocation to verify randVal is not
nullptr before dereferencing it, and if it is nullptr, handle the error
appropriately by returning early or setting an error status. Apply similar null
checks for other pointers like partVal and rv throughout the file to prevent
unsafe dereferences.
| LocalPointer<InternalValue> partVal( | ||
| formatExpression(globalEnv, part.contents(), context, status)); | ||
| FormattedPlaceholder partResult = partVal->forceFormatting(context.getErrors(), | ||
| status); | ||
| // Force full evaluation, e.g. applying default formatters to | ||
| // unformatted input (or formatting numbers as strings) | ||
| UnicodeString partResult = partVal.formatToString(locale, status); | ||
| result += partResult; | ||
| result += partResult.formatToString(locale, status); | ||
| // Handle formatting errors. `formatToString()` can't take a context and thus can't |
There was a problem hiding this comment.
Missing null check before forceFormatting()
partVal may be nullptr on allocation failure but is dereferenced unconditionally. The caller only checks U_FAILURE(status); add a pointer check to avoid UB.
🤖 Prompt for AI Agents
In deps/icu-small/source/i18n/messageformat2.cpp around lines 283 to 290, the
pointer partVal may be nullptr if allocation fails, but forceFormatting() is
called on it without checking. Add a null check for partVal before calling
partVal->forceFormatting() to prevent undefined behavior, and handle the error
appropriately if partVal is null.
| U_EXPORT icu::CharString | ||
| ulocimp_getKeywords(const char* localeID, | ||
| ulocimp_getKeywords(std::string_view localeID, | ||
| char prev, | ||
| bool valuesToo, | ||
| UErrorCode& status); | ||
|
|
||
| U_EXPORT void | ||
| ulocimp_getKeywords(const char* localeID, | ||
| ulocimp_getKeywords(std::string_view localeID, | ||
| char prev, | ||
| icu::ByteSink& sink, | ||
| bool valuesToo, | ||
| UErrorCode& status); | ||
|
|
||
| U_EXPORT icu::CharString | ||
| ulocimp_getName(const char* localeID, | ||
| ulocimp_getName(std::string_view localeID, | ||
| UErrorCode& err); | ||
|
|
||
| U_EXPORT void | ||
| ulocimp_getName(const char* localeID, | ||
| ulocimp_getName(std::string_view localeID, | ||
| icu::ByteSink& sink, | ||
| UErrorCode& err); | ||
|
|
||
| U_EXPORT icu::CharString | ||
| ulocimp_getBaseName(const char* localeID, | ||
| ulocimp_getBaseName(std::string_view localeID, | ||
| UErrorCode& err); | ||
|
|
||
| U_EXPORT void | ||
| ulocimp_getBaseName(const char* localeID, | ||
| ulocimp_getBaseName(std::string_view localeID, | ||
| icu::ByteSink& sink, | ||
| UErrorCode& err); | ||
|
|
||
| U_EXPORT icu::CharString | ||
| ulocimp_canonicalize(const char* localeID, | ||
| ulocimp_canonicalize(std::string_view localeID, | ||
| UErrorCode& err); | ||
|
|
||
| U_EXPORT void | ||
| ulocimp_canonicalize(const char* localeID, | ||
| ulocimp_canonicalize(std::string_view localeID, | ||
| icu::ByteSink& sink, | ||
| UErrorCode& err); | ||
|
|
There was a problem hiding this comment.
C ABI broken by use of std::string_view in exported C symbols
All functions annotated with U_EXPORT/U_CAPI form the stable C interface. Switching from const char* to std::string_view changes the mangling and makes the symbols impossible to call from C, breaking binary compatibility.
Keep the C façade (const char*) and add separate C++ overloads that take std::string_view if desired.
🤖 Prompt for AI Agents
In deps/icu-small/source/common/ulocimp.h between lines 70 and 109, the exported
C API functions use std::string_view as parameters, which breaks C ABI
compatibility. To fix this, revert the function signatures annotated with
U_EXPORT/U_CAPI to use const char* instead of std::string_view for the localeID
parameter, preserving the stable C interface. Then, add separate C++ overloads
that accept std::string_view if needed, ensuring the original C symbols remain
unchanged and compatible.
| while (peek() != LEFT_CURLY_BRACE || isWhitespace(peek()) || isBidiControl(peek())) { | ||
| bool wasWhitespace = isWhitespace(peek()) || isBidiControl(peek()); | ||
| parseRequiredWhitespace(status); | ||
| if (!wasWhitespace) { | ||
| // Avoid infinite loop when parsing something like: |
There was a problem hiding this comment.
Loop condition allows non-whitespace before keys – parsing bug
while (peek() != LEFT_CURLY_BRACE || isWhitespace(peek()) || isBidiControl(peek()))
Because of the leading peek()!=LEFT_CURLY_BRACE, the loop executes even when the next char is a key character (not whitespace), causing parseRequiredWhitespace() to fire with no preceding whitespace → spurious syntax errors and the fallback next() hack.
Replace the condition with one that only iterates when the next token starts with whitespace:
while (isWhitespace(peek()) || isBidiControl(peek())) {
…
}This removes the need for the fragile wasWhitespace/next() workaround.
🤖 Prompt for AI Agents
In deps/icu-small/source/i18n/messageformat2_parser.cpp around lines 1779 to
1783, the loop condition incorrectly allows the loop to run when the next
character is not whitespace, causing parseRequiredWhitespace() to be called
improperly and leading to syntax errors. Change the loop condition to only
continue while the next character is whitespace or a bidi control character by
replacing the condition with while (isWhitespace(peek()) ||
isBidiControl(peek())). This also allows removal of the wasWhitespace flag and
the next() workaround inside the loop.
| while (localeID.front() == ' ') { | ||
| localeID.remove_prefix(1); | ||
| } | ||
| if (!*pos) { /* handle trailing "; " */ | ||
| if (localeID.empty()) { /* handle trailing "; " */ |
There was a problem hiding this comment.
Possible UB: .front() called on empty string_view
Inside the space-skipping loop:
while (localeID.front() == ' ') {
localeID.remove_prefix(1);
}If localeID contains only spaces, remove_prefix empties the view and the next front() dereferences an empty view (undefined behaviour).
Guard with an emptiness check:
-while (localeID.front() == ' ') {
+while (!localeID.empty() && localeID.front() == ' ') {🤖 Prompt for AI Agents
In deps/icu-small/source/common/uloc.cpp around lines 630 to 633, the loop
calling localeID.front() does not check if localeID is empty before accessing
front(), which can cause undefined behavior if localeID contains only spaces.
Fix this by adding a condition to the while loop to ensure localeID is not empty
before calling front(), for example, change the loop condition to check that
localeID is not empty and the front character is a space.
20 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Documentation
End-users benefit from expanded internationalization support, improved reliability and accuracy in date/time and unit formatting, and enhanced platform compatibility.