[PATCH] D68664: [lit] Clean up internal diff's encoding handling

Tue Oct 8 14:43:27 PDT 2019

jdenny marked 2 inline comments as done.
jdenny added a comment.

In D68664#1700456 <https://reviews.llvm.org/D68664#1700456>, @rnk wrote:

> I'm guessing you've tested with Python 2.7 and 3.5, and that's probably what matters.

I have 2.7.15 and 3.6.8, but I assume that's sufficient.  With each, I've run check-lit and manually tried diff.py.  I still need to try check-all before pushing everything.

> Thanks for working on this, and sorry for ever expanding scope of the suggested refactorings.

It all needed to be done.  I just ran out of time last month.  Thanks for the reviews.

================
Comment at: llvm/utils/lit/lit/builtin_commands/diff.py:36
+                                   locale.getpreferredencoding(False))
+    except UnicodeDecodeError:
+        try:
----------------
rnk wrote:
> try / except UnicodeDecodeError is an exciting code pattern, but I guess it's the existing behavior. :)
I'm not quite sure how to interpret this remark.  Indeed, it's the existing behavior.  Do you recommend an alternative?

================
Comment at: llvm/utils/lit/lit/builtin_commands/diff.py:43
+def compareTwoBinaryFiles(flags, filepaths, filelines):
+    #sys.stderr.write("Trying as binary....\n")
     exitCode = 0
----------------
rnk wrote:
> Extra logging?
Hmm.  I'm being sloppy.  I'll remove.  Thanks.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68664/new/

https://reviews.llvm.org/D68664