Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2488853002:
Enable /utf-8 on Windows (Closed)
|
Created:
4 years, 1 month ago by scottmg Modified:
4 years ago Reviewers:
brucedawson CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable /utf-8 on Windows
Applies only to chromium_code for now. There's some characters in
third_party that cl.exe claims aren't representable in code page 65001
(which is its way of saying UTF-8 without a BOM).
R=brucedawson@chromium.org
BUG=454858, 637203
Committed: https://crrev.com/63384a79de37f89a88cfc02ff61eb649e4abe535
Cr-Commit-Position: refs/heads/master@{#434212}
Patch Set 1 #Patch Set 2 : wip on message compiler #Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : utf8 #Messages
Total messages: 33 (22 generated)
Description was changed from ========== Enable /source-charset:utf-8 on Windows Applies only to chromium_code for now. There's some characters in third_party that cl.exe claims aren't representable in code page 65001 (which is its way of saying UTF-8 without a BOM). R=thakis@chromium.org BUG=454858,637203 ========== to ========== Enable /source-charset:utf-8 on Windows Applies only to chromium_code for now. There's some characters in third_party that cl.exe claims aren't representable in code page 65001 (which is its way of saying UTF-8 without a BOM). R=brucedawson@chromium.org BUG=454858,637203 ==========
scottmg@chromium.org changed reviewers: + brucedawson@chromium.org - thakis@chromium.org
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Oops, I'm a moron, I put this in the !is_win block and then diligently waited for a local build, which tested nothing. Ignore for now. :/
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
I think we can turn this on now, assuming the tryjobs come back happy.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm, assuming that you spot-checked that the C4566 warnings don't indicate actual non-UTF8 characters (there were some copyright symbols and others which I assume are now gone). Did you test with the CJK code page? Just curious. Thanks for pushing this through.
On 2016/11/22 22:35:33, brucedawson wrote: > lgtm, assuming that you spot-checked that the C4566 warnings don't indicate > actual non-UTF8 characters (there were some copyright symbols and others which I > assume are now gone). I had done an initial check, but that was when I was trying for all code, not just chromium_code. Now that it's only for chromium_code, I think there'll be not-that-many, so I'll remove it globally and suppress it more locally instead. (Do I misunderstand the warning? It's telling me that the given character can't be represented in 1252, but ... that was the whole point of setting this flag in the first place, so it seems sort of dumb.) > > Did you test with the CJK code page? Just curious. Nope, I didn't want to mess with my setup. Hopefully someone else will feel daring. > > Thanks for pushing this through.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Urgh, I totally mis-assumed what /source-charset and /execution-charset were based on their names. (I assumed the latter was for something at runtime, but it's just for the second phase of compilation). Additionally, /source-charset turns on /validate-charset, which effectively corrupts source code if you ignore 4566! (it replaces the things it deems invalid by literal ??), causing e.g. https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2... So, disabling 4566 is a poor idea. Back to the drawing board. I believe we really do need /utf-8 now then, if we're going to do anything.
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Enable /source-charset:utf-8 on Windows Applies only to chromium_code for now. There's some characters in third_party that cl.exe claims aren't representable in code page 65001 (which is its way of saying UTF-8 without a BOM). R=brucedawson@chromium.org BUG=454858,637203 ========== to ========== Enable /utf-8 on Windows Applies only to chromium_code for now. There's some characters in third_party that cl.exe claims aren't representable in code page 65001 (which is its way of saying UTF-8 without a BOM). R=brucedawson@chromium.org BUG=454858,637203 ==========
On 2016/11/22 23:52:18, scottmg wrote: > Urgh, I totally mis-assumed what /source-charset and /execution-charset were > based on their names. (I assumed the latter was for something at runtime, but > it's just for the second phase of compilation). > > Additionally, /source-charset turns on /validate-charset, which effectively > corrupts source code if you ignore 4566! (it replaces the things it deems > invalid by literal ??), causing e.g. > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2... > > So, disabling 4566 is a poor idea. Back to the drawing board. > > I believe we really do need /utf-8 now then, if we're going to do anything. OK, I made that more complicated than it needed to be. /utf-8 works fine, so switched to that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by scottmg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brucedawson@chromium.org Link to the patchset: https://codereview.chromium.org/2488853002/#ps80001 (title: "utf8")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1479926528052650,
"parent_rev": "71907217914b0282b7bcf4aa6fe00969de9dd230", "commit_rev":
"f98b46a2d1d2cffbd76a2048f07de15f613df927"}
Message was sent while issue was closed.
Description was changed from ========== Enable /utf-8 on Windows Applies only to chromium_code for now. There's some characters in third_party that cl.exe claims aren't representable in code page 65001 (which is its way of saying UTF-8 without a BOM). R=brucedawson@chromium.org BUG=454858,637203 ========== to ========== Enable /utf-8 on Windows Applies only to chromium_code for now. There's some characters in third_party that cl.exe claims aren't representable in code page 65001 (which is its way of saying UTF-8 without a BOM). R=brucedawson@chromium.org BUG=454858,637203 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Enable /utf-8 on Windows Applies only to chromium_code for now. There's some characters in third_party that cl.exe claims aren't representable in code page 65001 (which is its way of saying UTF-8 without a BOM). R=brucedawson@chromium.org BUG=454858,637203 ========== to ========== Enable /utf-8 on Windows Applies only to chromium_code for now. There's some characters in third_party that cl.exe claims aren't representable in code page 65001 (which is its way of saying UTF-8 without a BOM). R=brucedawson@chromium.org BUG=454858,637203 Committed: https://crrev.com/63384a79de37f89a88cfc02ff61eb649e4abe535 Cr-Commit-Position: refs/heads/master@{#434212} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/63384a79de37f89a88cfc02ff61eb649e4abe535 Cr-Commit-Position: refs/heads/master@{#434212} |
