[PATCH] D30773: Make git-clang-format python 3 compatible

[Eric Fiselier via Phabricator via llvm-commits]

EricWF marked 4 inline comments as done.
EricWF added inline comments.


================
Comment at: tools/clang-format/git-clang-format:323
   allowed_extensions = frozenset(allowed_extensions)
-  for filename in dictionary.keys():
+  for filename in list(dictionary.keys()):
     base_ext = filename.rsplit('.', 1)
----------------
kimgr wrote:
> This should not be necessary for iteration -- in Python3 it returns a generator instead of a list, but generators can be iterated.
This was done by the 2to3 tool. I'll have to see what it thinks it was up to. But I agree this seems wrong.


================
Comment at: tools/clang-format/git-clang-format:492
+      print(('The following files would be modified but '
+                           'have unstaged changes:'), file=sys.stderr)
+      print(unstaged_files, file=sys.stderr)
----------------
kimgr wrote:
> I wonder if `file=sys.stderr` works with Python 2.7's print statement. Have you tested this with 2.7?
> 
> You can use `__future__` to get the print function behavior in 2.7 as well:
> http://stackoverflow.com/questions/32032697/how-to-use-from-future-import-print-function
It doesn't. I think getting the new behavior from `__future__` is the only reasonable way to go.


================
Comment at: tools/clang-format/git-clang-format:512-515
+def to_string(bytes):
+    if isinstance(bytes, str):
+        return bytes
+    return to_bytes(bytes)
----------------
kimgr wrote:
> This looks wrong -- where does `to_bytes` come from?
> 
> I can never get my head around Python2's string/bytes philosophy. In Python3 you would do:
> 
>     # stdout and stderr are always `bytes`
>     stdout = stdout.decode('utf-8')
>     stderr = stderr.decode('utf-8')
> 
> (assuming the input *is* utf-8)
> 
> Not sure how that plays with Python2, but the current code doesn't look right.
> 
This is wrong. And it will be removed in the next set of changes.


https://reviews.llvm.org/D30773