<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 9, 2017 at 9:36 AM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Consider landing the obvious bits (print function, mostly) separately and use this only for the interesting bits.</div></blockquote><div><br></div><div>Sounds good. I just wanted to make sure I got the simple bits right as well.</div><div><br></div><div>If this doesn't get reviewed in the next week I'll start landing bits separately as suggested.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Thu, Mar 9, 2017 at 11:10 AM, Michał Górny via Phabricator via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">mgorny added inline comments.<br>
<br>
<br>
================<br>
Comment at: tools/clang-format/git-clang-f<wbr>ormat:293<br>
<br>
+def to_bytes(str):<br>
+    # Encode to UTF-8 to get binary data.<br>
----------------<br>
Pretty much a nit but using variable names that match type names can be a bit confusing here.<br>
<br>
<br>
================<br>
Comment at: tools/clang-format/git-clang-f<wbr>ormat:302<br>
<span>+        return bytes<br>
+    return to_bytes(bytes)<br>
</span>+<br>
----------------<br>
Shouldn't this be:<br>
<br>
    return bytes.decode('utf-8')<br>
<br>
?<br>
<br>
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.<br>
<br>
<br>
================<br>
Comment at: tools/clang-format/git-clang-f<wbr>ormat:306<br>
+    try:<br>
+        return to_string(bytes.decode('utf-8'<wbr>))<br>
+    except AttributeError: # 'str' object has no attribute 'decode'.<br>
----------------<br>
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?<br>
<br>
<br>
================<br>
Comment at: tools/clang-format/git-clang-f<wbr>ormat:310<br>
+    except UnicodeError:<br>
+        return str(bytes)<br>
+<br>
----------------<br>
I don't think this can ever succeed. If the argument is not valid utf8, it certainly won't be valid ASCII.<br>
<span><br>
<br>
================<br>
Comment at: tools/clang-format/git-clang-f<wbr>ormat:323<br>
</span>   for line in patch_file:<br>
-    match = re.search(r'^\+\+\+\ [^/]+/(.*)', line)<br>
+    match = re.search(to_bytes(r'^\+\+\+\ [^/]+/(.*)'), line)<br>
     if match:<br>
----------------<br>
Any reason not to use:<br>
<br>
    br'^\+...'<br>
<br>
? i.e. make it bytes immediately instead of converting.<br>
<br>
<br>
================<br>
Comment at: tools/clang-format/git-clang-f<wbr>ormat:344<br>
+  keys_to_delete = []<br>
<span>+  for filename in list(dictionary.keys()):<br>
</span>+    filename_cp = convert_string(bytes(filename)<wbr>)<br>
----------------<br>
Since you are using `keys_to_delete` now, you can remove the `list()`.<br>
<br>
<br>
================<br>
Comment at: tools/clang-format/git-clang-f<wbr>ormat:348<br>
     if len(base_ext) == 1 or base_ext[1].lower() not in allowed_extensions:<br>
-      del dictionary[filename]<br>
+      keys_to_delete += [filename]<br>
+  for key in keys_to_delete:<br>
----------------<br>
Another nit. I think it'd be better to just append a single item instead of a list of 1 item ;-).<br>
<br>
    keys_to_delete.append(filename<wbr>)<br>
</div></div><div class="m_-4831860965527502298HOEnZb"><div class="m_-4831860965527502298h5"><br>
<br>
<a href="https://reviews.llvm.org/D30773" rel="noreferrer" target="_blank">https://reviews.llvm.org/D3077<wbr>3</a><br>
<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div>
</blockquote></div><br></div></div>