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



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

Reply via email to