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

[Nico Weber via llvm-commits]

Consider landing the obvious bits (print function, mostly) separately and
use this only for the interesting bits.

On Thu, Mar 9, 2017 at 11:10 AM, Michał Górny via Phabricator via
cfe-commits <cfe-commits at lists.llvm.org> wrote:

> 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 at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170309/77165f5b/attachment.html>