On Thu, Mar 9, 2017 at 9:36 AM, Nico Weber <tha...@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@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@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
