- User Since
- Aug 27 2014, 8:34 PM (267 w, 6 d)
I don't think that this is a clear "win". Users may have UTF-8 capable locales not ending in ".UTF-8" and also not have a usable en_US.UTF-8 locale. It might be nice for some people, but this can actually break others. I don't see an issue with asking users to specify an appropriate value of LANG for their environment.
Sat, Oct 12
Fri, Oct 11
LGTM with minor changes.
Thanks @DiggerLin. I think this is almost ready.
I've marked comments (all minor) that have not yet been addressed.
LGTM with minor changes that can be made on the check-in.
Thu, Oct 10
Just some minor comments. I think this is almost ready.
LGTM with some straightforward changes (can be fixed on the commit).
Wed, Oct 9
LGTM to land as-is. Not sure if other people have an opinion about the const.
@DiggerLin, I believe you have had a number of patches committed into the project. I think you can request commit access and land this yourself. Thanks.
Tue, Oct 8
LGTM. Thanks again.
Thanks @DiggerLin. Just minor comments.
Mon, Oct 7
Thanks for adding the BOM. With the BOM, would it make sense to leave mri-utf8.test as the name of the file?
My understanding is that this patch has no effect for Python 3. In Python 2, object from builtins (as provided by the future package) is used to enable use of some Python 3 coding patterns. Absent further changes that make use of such enablement, I am not sure that this patch is necessary. If this patch is needed to support later patches, then I suggest applying the fixer at that point. Applying the fixer before applying D67882 seems odd anyway.
Perhaps others are more well-versed in this than I am, but I think that having a link in the commit message and using the terminology used by the documentation ("new-style" and "classic") would be useful here: https://docs.python.org/2/reference/datamodel.html#newstyle. Also, I am not sure that switching these to be new-style classes in Python 2 is necessary. I believe the commit message should give additional rationale, e.g., using new-style classes helps make the Python 2 and Python 3 behaviour of the code more similar.
Sun, Oct 6
Sat, Oct 5
Fri, Oct 4
There's an en_US.UTF-8 on "my" AIX box and an en_US.utf8 on "my" RHEL 7 box. There's no "C" UTF-8 locale anywhere in sight.
Thu, Oct 3
Wed, Oct 2
LGTM. New declarations of IsDarwin in this patch are in contexts where there is no existing reference to any IsDarwin that might have been in scope.
Mon, Sep 30
My general impression is that this is harmless. As it is, there is no other key available to sort on. @cmatthews, can you confirm?
Sun, Sep 29
Sat, Sep 28
Just following up on my previous comments on this patch based on developments in D66969.
Fri, Sep 27
It seems to be a separable feature (although it does interact with -ftrivial-auto-init=pattern). That option also provides guardrails for non-unions, and "bumper guardrails" for unions can be a useful feature without the non-union guardrails.
I agree, and guardrails have a tendency to scratch paint if one drives against them.
Thu, Sep 26
Wed, Sep 25
I might have more comments later, but I'm posting what I have currently.
Tue, Sep 24
LGTM with suggestion for a minor drive-by typo correction.
LGTM with suggestion to update commit message.
LGTM with comments on further changes that could be made.
Mon, Sep 23
Sat, Sep 21