hokein added inline comments. ================ Comment at: clang-rename/tool/clang-rename.el:16 @@ +15,3 @@ + +(defvar clang-rename-binary "clang-rename") + ---------------- I think we should make `clang-rename` binary path configurable by making it a custom variable (using `defcustom`).
================ Comment at: clang-rename/tool/clang-rename.el:20 @@ +19,3 @@ + "Rename all instances of the symbol at the point using clang-rename" + (interactive "sEnter a new name: ") + (let (;; Emacs offset is 1-based. ---------------- `s` is an extra character here? ================ Comment at: clang-rename/tool/clang-rename.el:27 @@ +26,3 @@ + (let ((rename-command + (format "bash -f -c '%s -offset=%s -new-name=%s -i %s'" + clang-rename-binary offset new-name file-name))) ---------------- Any reason why not use `call-process-region`? ================ Comment at: docs/clang-rename.rst:106 @@ +105,3 @@ +clang-rename Emacs integration +============================ + ---------------- missing two "=". ================ Comment at: docs/clang-rename.rst:114 @@ +113,3 @@ +Once installed, you can point your cursor to symbols you want to rename, press +`M-X`, print `clang-rename` and new desired name. + ---------------- `print` doesn't make sense here? I think user should type `clang-rename` and new name. https://reviews.llvm.org/D23006 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits