phst added a comment.

Thanks, generally looks good, but a couple of notes:

- This is actually a bug in clang-format, which can easily be triggered with 
`clang-format -output-replacements-xml -assume-filename '' -offset 0 -length 10 
-cursor 5 <<< 'aaaa = 1234;'`

  It's fine to work around bugs, but please file a bug against clang-format 
itself and reference it in a comment.

- Not sure whether the `assume-filename` parameter is actually needed. If 
there's no known use case, please don't add it ("YAGNI").

- Prefer quoting function symbols with `#'`: `#'call-process-region`. The 
byte-compiler will then warn if such a symbol is not defined as a function.

- It's more readable to write the non-varying arguments to call-process-region 
as direct arguments to `apply`, like so: `(apply #'call-process-region nil ... 
nil (append '("-output-replacements ...`. The result is equivalent, but this 
structure makes it more obvious which arguments are positional and which are 
rest arguments.

- You can combine the various lists with backquotes and get rid of `append`: 
``("-output-replacements-xml" ,@(and buffer-file-name (list (concat 
"-assume-filename=" buffer-file-name))) "-style" ...)`


https://reviews.llvm.org/D37903



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to