[PATCH] D71110: [clangd] A tool to evaluate cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

sorry for the delay, I just saw the comment today.

In D71110#1799960 <https://reviews.llvm.org/D71110#1799960>, @kbobyrev wrote:

> For some reason, running this patch on a recent version of the source tree fails. The AST parsing fails with fatal diagnostics, but I can't understand why:
>
>   E[15:14:33.084] source file unknown type name 'PathRef' is illformed: 1
>   E[15:15:02.831] source file unknown type name 'NestedNameSpecifierLoc'; did you mean 'std::clang::NestedNameSpecifierLoc'? is illformed: 1
>   E[15:14:02.912] source file unknown type name 'Location' is illformed: 1
>   E[15:14:47.797] source file 'index' is not a class, namespace, or enumeration is illformed: 1
>   E[15:14:17.658] source file no template named 'vector' in namespace 'std' is illformed: 1
>


I encountered the same error when rebasing to master. It was work before. To parse the file correctly, we need to inject the clang builtin header directory (via `resource-dir`) to the compile command, it was done by OverlayCDB, but there was a patch changing this behavior in December, which broke the tool.
It should be fixed now.



================
Comment at: clang-tools-extra/clangd/eval-rename/eval-rename.py:1
+#!/usr/bin/python
+"""
----------------
kbobyrev wrote:
> It probably makes sense to migrate this to Python 3 instead (shouldn't be too hard, from what I can see there are Python 2 print statements, but nothing else I can find).
good point.


================
Comment at: clang-tools-extra/clangd/eval-rename/eval-rename.py:21
+
+RENAME_EXECUTOR='build/bin/clangd-rename'
+
----------------
kbobyrev wrote:
> Would be useful to add an argument to the build directory, I typically don't put `build/` into `llvm/`.
yeah. the current script currently assumes the directory layout:

```
llvm-project
  - build/
    - bin/...
  - clang-tools-extra
  - ...
```


================
Comment at: clang-tools-extra/clangd/eval-rename/eval-rename.py:82
+    success_cnt += 1
+  Run(['git', 'reset', '--hard'])
+  print 'Evaluated rename on %d symbols, %d symbol succeeded, go %s for failure logs' % (
----------------
kbobyrev wrote:
> `git reset --hard` might be dangerous if compile-commands are symlinked to the build directory: `ninja -C build` would re-generate them and `git reset --hard` will e.g. erase `add_subdirectory(eval-rename)` if it's not committed. Maybe this should be mentioned somewhere in the comments/user guide.
agree. it is dangerous, the script is expected to be ran on a clean client. we could improve it, e.g. prompt a confirm dialog, or abandon if  there are dirty changes in the client. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71110/new/

https://reviews.llvm.org/D71110