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

[Michał Górny via Phabricator via llvm-commits]

mgorny added inline comments.


================
Comment at: tools/clang-format/git-clang-format:293
 
+def to_bytes(str):
+    # Encode to UTF-8 to get binary data.
----------------
Pretty much a nit but using variable names that match type names can be a bit confusing here.


================
Comment at: tools/clang-format/git-clang-format:302
+        return bytes
+    return to_bytes(bytes)
+
----------------
Shouldn't this be:

    return bytes.decode('utf-8')

?

Otherwise, unless I'm missing something this function will always return the parameter [with no modifications], either in the first conditional if it's of `str` type, or in the conditional inside `to_bytes()` if it's of `bytes` type.


================
Comment at: tools/clang-format/git-clang-format:306
+    try:
+        return to_string(bytes.decode('utf-8'))
+    except AttributeError: # 'str' object has no attribute 'decode'.
----------------
This logic looks really weird to me. What is the purpose of having both `to_string()` and `convert_string()`? Why do `to_bytes()` and `to_string()` use `isinstance()` to recognize types, and here you rely on exceptions? Why is `to_string()` called after decoding?


================
Comment at: tools/clang-format/git-clang-format:310
+    except UnicodeError:
+        return str(bytes)
+
----------------
I don't think this can ever succeed. If the argument is not valid utf8, it certainly won't be valid ASCII.


================
Comment at: tools/clang-format/git-clang-format:323
   for line in patch_file:
-    match = re.search(r'^\+\+\+\ [^/]+/(.*)', line)
+    match = re.search(to_bytes(r'^\+\+\+\ [^/]+/(.*)'), line)
     if match:
----------------
Any reason not to use:

    br'^\+...'

? i.e. make it bytes immediately instead of converting.


================
Comment at: tools/clang-format/git-clang-format:344
+  keys_to_delete = []
+  for filename in list(dictionary.keys()):
+    filename_cp = convert_string(bytes(filename))
----------------
Since you are using `keys_to_delete` now, you can remove the `list()`.


================
Comment at: tools/clang-format/git-clang-format:348
     if len(base_ext) == 1 or base_ext[1].lower() not in allowed_extensions:
-      del dictionary[filename]
+      keys_to_delete += [filename]
+  for key in keys_to_delete:
----------------
Another nit. I think it'd be better to just append a single item instead of a list of 1 item ;-).

    keys_to_delete.append(filename)


https://reviews.llvm.org/D30773