Add flag to revert ICU 76 changes for commas in en-AU/GB/IN [v8/v8 : main]

0 views
Skip to first unread message

Philip Jägenstedt (Gerrit)

unread,
Sep 24, 2025, 11:15:34 AM (2 days ago) Sep 24
to Leszek Swirski, V8 LUCI CQ, v8-flag...@chromium.org, v8-re...@googlegroups.com
Attention needed from Leszek Swirski

Philip Jägenstedt added 1 comment

File src/objects/js-date-time-format.cc
Line 1733, Patchset 1 (Latest): (result.startsWith("Monday,") || result.startsWith("Tuesday,") ||
Philip Jägenstedt . unresolved

Yes, this is fairly "ick" but there are two reasons. First, it wasn't easy at this point in the code to get access to the input dateStyle to check if it's "full". Second, doing it this way is the most conservative and doesn't risk changing anything other than a comma following a full weekday.

Open in Gerrit

Related details

Attention is currently required from:
  • Leszek Swirski
Submit Requirements:
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: v8/v8
Gerrit-Branch: main
Gerrit-Change-Id: Iad5d8f498f15b771bf521e0bffda44105ce03a37
Gerrit-Change-Number: 6979345
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Jägenstedt <foo...@chromium.org>
Gerrit-Reviewer: Leszek Swirski <les...@chromium.org>
Gerrit-Reviewer: Philip Jägenstedt <foo...@chromium.org>
Gerrit-Attention: Leszek Swirski <les...@chromium.org>
Gerrit-Comment-Date: Wed, 24 Sep 2025 15:15:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
unsatisfied_requirement
open
diffy

Leszek Swirski (Gerrit)

unread,
Sep 24, 2025, 12:25:43 PM (2 days ago) Sep 24
to Philip Jägenstedt, V8 LUCI CQ, v8-flag...@chromium.org, v8-re...@googlegroups.com
Attention needed from Philip Jägenstedt

Leszek Swirski voted and added 1 comment

Votes added by Leszek Swirski

Code-Review+1

1 comment

File src/objects/js-date-time-format.cc
Line 1733, Patchset 1: (result.startsWith("Monday,") || result.startsWith("Tuesday,") ||
Philip Jägenstedt . unresolved

Yes, this is fairly "ick" but there are two reasons. First, it wasn't easy at this point in the code to get access to the input dateStyle to check if it's "full". Second, doing it this way is the most conservative and doesn't risk changing anything other than a comma following a full weekday.

Leszek Swirski

I'm not too familiar with ICU, but glancing over the API, would something like `date_format.toPattern().startsWith("EEEE,")` work? Or is it bad because of allocation? Either way, probably your review comment here should be a code comment.

Open in Gerrit

Related details

Attention is currently required from:
  • Philip Jägenstedt
Submit Requirements:
  • requirement satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: v8/v8
Gerrit-Branch: main
Gerrit-Change-Id: Iad5d8f498f15b771bf521e0bffda44105ce03a37
Gerrit-Change-Number: 6979345
Gerrit-PatchSet: 2
Gerrit-Owner: Philip Jägenstedt <foo...@chromium.org>
Gerrit-Reviewer: Leszek Swirski <les...@chromium.org>
Gerrit-Reviewer: Philip Jägenstedt <foo...@chromium.org>
Gerrit-Attention: Philip Jägenstedt <foo...@chromium.org>
Gerrit-Comment-Date: Wed, 24 Sep 2025 16:25:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Philip Jägenstedt <foo...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Philip Jägenstedt (Gerrit)

unread,
Sep 25, 2025, 12:06:54 PM (24 hours ago) Sep 25
to Leszek Swirski, V8 LUCI CQ, v8-flag...@chromium.org, v8-re...@googlegroups.com
Attention needed from Leszek Swirski

Philip Jägenstedt added 1 comment

File src/objects/js-date-time-format.cc
Line 1733, Patchset 1: (result.startsWith("Monday,") || result.startsWith("Tuesday,") ||
Philip Jägenstedt . unresolved

Yes, this is fairly "ick" but there are two reasons. First, it wasn't easy at this point in the code to get access to the input dateStyle to check if it's "full". Second, doing it this way is the most conservative and doesn't risk changing anything other than a comma following a full weekday.

Leszek Swirski

I'm not too familiar with ICU, but glancing over the API, would something like `date_format.toPattern().startsWith("EEEE,")` work? Or is it bad because of allocation? Either way, probably your review comment here should be a code comment.

Philip Jägenstedt

Great suggestion, I didn't know I could get the pattern out here, but it works.

I also went looking for a place where the pattern is first chosen to see if I could change it there, but it turns out CreateICUDateFormat isn't called as part of the test I wrote. I stepped around in a debugger and couldn't find a place where we have the pattern, so I think it's created inside of ICU and isn't extracted elsewhere in all situations.

I put the toPattern() bit last and explained why. The hope is that this code is never enabled, it's only for an emergency and would then be short lived.

Open in Gerrit

Related details

Attention is currently required from:
  • Leszek Swirski
Submit Requirements:
  • requirement satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: v8/v8
Gerrit-Branch: main
Gerrit-Change-Id: Iad5d8f498f15b771bf521e0bffda44105ce03a37
Gerrit-Change-Number: 6979345
Gerrit-PatchSet: 3
Gerrit-Owner: Philip Jägenstedt <foo...@chromium.org>
Gerrit-Reviewer: Leszek Swirski <les...@chromium.org>
Gerrit-Reviewer: Philip Jägenstedt <foo...@chromium.org>
Gerrit-Attention: Leszek Swirski <les...@chromium.org>
Gerrit-Comment-Date: Thu, 25 Sep 2025 16:06:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Philip Jägenstedt <foo...@chromium.org>
Comment-In-Reply-To: Leszek Swirski <les...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Philip Jägenstedt (Gerrit)

unread,
Sep 25, 2025, 12:31:42 PM (23 hours ago) Sep 25
to Michael Lippautz, Leszek Swirski, V8 LUCI CQ, v8-flag...@chromium.org, v8-re...@googlegroups.com
Attention needed from Leszek Swirski and Michael Lippautz

Philip Jägenstedt voted and added 1 comment

Votes added by Philip Jägenstedt

Auto-Submit+1
Commit-Queue+1

1 comment

Patchset-level comments
File-level comment, Patchset 3 (Latest):
Philip Jägenstedt . resolved

I'll need two LGTMs on this, and my changes invalidated the first one :)

Open in Gerrit

Related details

Attention is currently required from:
  • Leszek Swirski
  • Michael Lippautz
Submit Requirements:
  • requirement satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: v8/v8
Gerrit-Branch: main
Gerrit-Change-Id: Iad5d8f498f15b771bf521e0bffda44105ce03a37
Gerrit-Change-Number: 6979345
Gerrit-PatchSet: 3
Gerrit-Owner: Philip Jägenstedt <foo...@chromium.org>
Gerrit-Reviewer: Leszek Swirski <les...@chromium.org>
Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Philip Jägenstedt <foo...@chromium.org>
Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>
Gerrit-Attention: Leszek Swirski <les...@chromium.org>
Gerrit-Comment-Date: Thu, 25 Sep 2025 16:31:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Leszek Swirski (Gerrit)

unread,
Sep 25, 2025, 12:55:59 PM (23 hours ago) Sep 25
to Philip Jägenstedt, Michael Lippautz, V8 LUCI CQ, v8-flag...@chromium.org, v8-re...@googlegroups.com
Attention needed from Michael Lippautz and Philip Jägenstedt

Leszek Swirski voted Code-Review+1

Code-Review+1
Open in Gerrit

Related details

Attention is currently required from:
  • Michael Lippautz
  • Philip Jägenstedt
Submit Requirements:
  • requirement satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: v8/v8
Gerrit-Branch: main
Gerrit-Change-Id: Iad5d8f498f15b771bf521e0bffda44105ce03a37
Gerrit-Change-Number: 6979345
Gerrit-PatchSet: 3
Gerrit-Owner: Philip Jägenstedt <foo...@chromium.org>
Gerrit-Reviewer: Leszek Swirski <les...@chromium.org>
Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Philip Jägenstedt <foo...@chromium.org>
Gerrit-Attention: Philip Jägenstedt <foo...@chromium.org>
Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>
Gerrit-Comment-Date: Thu, 25 Sep 2025 16:55:54 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Michael Lippautz (Gerrit)

unread,
3:15 AM (9 hours ago) 3:15 AM
to Philip Jägenstedt, Leszek Swirski, V8 LUCI CQ, v8-flag...@chromium.org, v8-re...@googlegroups.com
Attention needed from Philip Jägenstedt

Michael Lippautz voted and added 1 comment

Votes added by Michael Lippautz

Code-Review+1

1 comment

Patchset-level comments
Michael Lippautz . resolved

stmap

Open in Gerrit

Related details

Attention is currently required from:
  • Philip Jägenstedt
Submit Requirements:
  • requirement satisfiedCode-Owners
  • requirement satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: v8/v8
Gerrit-Branch: main
Gerrit-Change-Id: Iad5d8f498f15b771bf521e0bffda44105ce03a37
Gerrit-Change-Number: 6979345
Gerrit-PatchSet: 3
Gerrit-Owner: Philip Jägenstedt <foo...@chromium.org>
Gerrit-Reviewer: Leszek Swirski <les...@chromium.org>
Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Philip Jägenstedt <foo...@chromium.org>
Gerrit-Attention: Philip Jägenstedt <foo...@chromium.org>
Gerrit-Comment-Date: Fri, 26 Sep 2025 07:15:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Philip Jägenstedt (Gerrit)

unread,
3:57 AM (8 hours ago) 3:57 AM
to Michael Lippautz, Leszek Swirski, V8 LUCI CQ, v8-flag...@chromium.org, v8-re...@googlegroups.com

Philip Jägenstedt voted and added 1 comment

Votes added by Philip Jägenstedt

Commit-Queue+2

1 comment

File src/objects/js-date-time-format.cc
Line 1733, Patchset 1: (result.startsWith("Monday,") || result.startsWith("Tuesday,") ||
Philip Jägenstedt . resolved

Yes, this is fairly "ick" but there are two reasons. First, it wasn't easy at this point in the code to get access to the input dateStyle to check if it's "full". Second, doing it this way is the most conservative and doesn't risk changing anything other than a comma following a full weekday.

Leszek Swirski

I'm not too familiar with ICU, but glancing over the API, would something like `date_format.toPattern().startsWith("EEEE,")` work? Or is it bad because of allocation? Either way, probably your review comment here should be a code comment.

Philip Jägenstedt

Great suggestion, I didn't know I could get the pattern out here, but it works.

I also went looking for a place where the pattern is first chosen to see if I could change it there, but it turns out CreateICUDateFormat isn't called as part of the test I wrote. I stepped around in a debugger and couldn't find a place where we have the pattern, so I think it's created inside of ICU and isn't extracted elsewhere in all situations.

I put the toPattern() bit last and explained why. The hope is that this code is never enabled, it's only for an emergency and would then be short lived.

Philip Jägenstedt

Marked as resolved.

Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
    • requirement satisfiedCode-Owners
    • requirement satisfiedCode-Review
    • requirement satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: v8/v8
    Gerrit-Branch: main
    Gerrit-Change-Id: Iad5d8f498f15b771bf521e0bffda44105ce03a37
    Gerrit-Change-Number: 6979345
    Gerrit-PatchSet: 3
    Gerrit-Owner: Philip Jägenstedt <foo...@chromium.org>
    Gerrit-Reviewer: Leszek Swirski <les...@chromium.org>
    Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
    Gerrit-Reviewer: Philip Jägenstedt <foo...@chromium.org>
    Gerrit-Comment-Date: Fri, 26 Sep 2025 07:57:52 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    open
    diffy

    V8 LUCI CQ (Gerrit)

    unread,
    3:59 AM (8 hours ago) 3:59 AM
    to Philip Jägenstedt, Michael Lippautz, Leszek Swirski, v8-flag...@chromium.org, v8-re...@googlegroups.com

    V8 LUCI CQ submitted the change

    Change information

    Commit message:
    Add flag to revert ICU 76 changes for commas in en-AU/GB/IN
    Bug: 421834885
    Change-Id: Iad5d8f498f15b771bf521e0bffda44105ce03a37
    Auto-Submit: Philip Jägenstedt <foo...@chromium.org>
    Reviewed-by: Michael Lippautz <mlip...@chromium.org>
    Commit-Queue: Philip Jägenstedt <foo...@chromium.org>
    Reviewed-by: Leszek Swirski <les...@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#102780}
    Files:
    • M src/flags/flag-definitions.h
    • M src/objects/js-date-time-format.cc
    • A test/intl/number-format/comma-compat-off.js
    • A test/intl/number-format/comma-compat-on.js
    Change size: M
    Delta: 4 files changed, 69 insertions(+), 0 deletions(-)
    Branch: refs/heads/main
    Submit Requirements:
    • requirement satisfiedCode-Review: +1 by Michael Lippautz, +1 by Leszek Swirski
    Open in Gerrit
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: merged
    Gerrit-Project: v8/v8
    Gerrit-Branch: main
    Gerrit-Change-Id: Iad5d8f498f15b771bf521e0bffda44105ce03a37
    Gerrit-Change-Number: 6979345
    Gerrit-PatchSet: 4
    Gerrit-Owner: Philip Jägenstedt <foo...@chromium.org>
    Gerrit-Reviewer: Leszek Swirski <les...@chromium.org>
    Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
    Gerrit-Reviewer: Philip Jägenstedt <foo...@chromium.org>
    open
    diffy
    satisfied_requirement
    Reply all
    Reply to author
    Forward
    0 new messages