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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

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've had many unsuccessful attempts to track down the bug and use multiple different configurations (i.e. I thought something might be wrong with my index location or directory structure, so in the end I replicated the setup that I can infer from the code), but I still didn't manage to run the renames.

In order to make it easier for you to understand what went wrong I have logged my commands and output so that one could understand what happens. I've tried several versions of LLVM, the log is for the most recent one, https://github.com/kirillbobyrev/llvm-project/commit/c7dc4734d23f45f576ba5af2aae5be9dfa2d3643. I've made sure to run `check-all` to validate correctness of source tree and to generate all test files so that indexer does not crash. I've made sure that all the commands ran without any errors.

Because the output is quite verbose, I'm submitting another file which only lists the commands instead of the one with verbose output. If you need the full log, please let me know. Attached file: F11162505: log.txt <https://reviews.llvm.org/F11162505>

Please let me know if I have done something incorrectly.



================
Comment at: clang-tools-extra/clangd/eval-rename/RenameMain.cpp:34
+static llvm::cl::opt<bool>
+    Fix("fix", llvm::cl::desc("Apply the rename edits to disk files"),
+        llvm::cl::init(false));
----------------
Maybe just `Apply`? `Fix` seems slightly confusing.


================
Comment at: clang-tools-extra/clangd/eval-rename/eval-rename.py:1
+#!/usr/bin/python
+"""
----------------
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).


================
Comment at: clang-tools-extra/clangd/eval-rename/eval-rename.py:11
+$ ninja -C build clangd-rename
+$ clang-tools-extra/clangd/eval-rename/eval-rename.py --index=llvm-index.idx --file=clang-tools-extra/clangd/eval-rename/symbol_to_rename.txt
+"""
----------------
Maybe some comments regarding what each field in `symbol_to_rename.txt` corresponds to and how it is parsed would be useful.


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


================
Comment at: clang-tools-extra/clangd/eval-rename/eval-rename.py:23
+
+def Run(cmd):
+  s = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
----------------
nit: `run`?


================
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' % (
----------------
`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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71110