LGTM. Thanks.

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.

Committed: https://github.com/llvm/llvm-lnt/commit/f805c3e1eece29e502a88a095c710f32473ca3c5

LGTM.

LGTM with minor changes.

LGTM. Thanks.

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.

Just some minor comments. I think this is almost ready.

LGTM with some straightforward changes (can be fixed on the commit).

LGTM.

LGTM.

LGTM.

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.

LGTM. Thanks again.

Thanks @DiggerLin. Just minor comments.

In D67882#1697616, @thopre wrote:

I think we should make LNT Python 3 only as soon as it can work in that mode, so the less compability code we can add the better. Do you agree with that approach?

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.

LGTM.

LGTM.

LGTM.

In D68472#1695156, @thopre wrote:

I believe that's because it is a builtin locale (much like the C locale). There wasn't one in /usr/share/locale on the Ubuntu docker image I've tested this but it did work while trying with en_US.UTF-8 did not.

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.

LGTM.

LGTM.

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.

LGTM.

My general impression is that this is harmless. As it is, there is no other key available to sort on. @cmatthews, can you confirm?

LGTM.

Just following up on my previous comments on this patch based on developments in D66969.

In D66969#1683070, @DiggerLin wrote:

we can not generate a raw data of the '.text" XCOFF section.
there are upcoming patch which deal with the DS csect , I think we will add the test in that patch @jasonliu.

In D68115#1686887, @vitalybuka wrote:

I would be happy to update the patch to enable it only for -ftrivial-auto-var-init=pattern, if we want "bumper" version.

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.

In D68115#1686837, @jfb wrote:

The entire point of this feature is to add guardrails to the language.

I agree, and guardrails have a tendency to scratch paint if one drives against them.

LGTM.

In D67884#1678058, @thopre wrote:

Alternatively the test could use -DAG directives. But I do think sorting is nicer as it's easier to find a machine in a long list and doing it on the server side sounds sensible to me. I'll let Chris have the last word though.

In D67853#1684295, @sfertile wrote:

Thanks for the explanation Hubert. I've updated the comments and committed.

LGTM. Thanks.

I might have more comments later, but I'm posting what I have currently.

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.

LGTM.

LGTM.

LGTM.

LGTM.

LGTM.

LGTM.