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

[Eric Fiselier via cfe-commits]

On Thu, Mar 9, 2017 at 9:36 AM, Nico Weber <thakis at chromium.org> wrote:

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

Sounds good. I just wanted to make sure I got the simple bits right as well.

If this doesn't get reviewed in the next week I'll start landing bits
separately as suggested.


>
> 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/cfe-commits/attachments/20170310/9311b489/attachment.html>