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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
