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@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