[PATCH] D33045: [libclang] Avoid more stats than necessary for reparse.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I've added a few more comments, but I'm not really helping much here. Someone more experienced with ASTUnit is certainly needed.
Also, please be sure to check clangd(from clang-tools-extra) before submitting, it's also a client of ASTUnit that might've breaked.



================
Comment at: include/clang/Frontend/ASTUnit.h:521
 
+  void recreateFileManager();
+
----------------
It would be really unfortunate to have that as a part of public API.


================
Comment at: lib/Frontend/ASTUnit.cpp:2074
   // Clear out the diagnostics state.
-  FileMgr.reset();
   getDiagnostics().Reset();
----------------
nik wrote:
> ilya-biryukov wrote:
> > Parse method used to recreate FileMgr from CompilerInvocation, because it was reset() at this point.
> > Won't something break because we're reusing the FileMgr now?
> > Maybe the code creating FileMgr in Parse should be deleted?
> > Parse method used to recreate FileMgr from CompilerInvocation, because it was reset() at this point.
> 
> It still does so for the initial parse (clang_parseTranslationUnit).
> 
> > Won't something break because we're reusing the FileMgr now?
> 
> Hmm, now that I've adapted the other callers I get failing tests :/
> 
> > Maybe the code creating FileMgr in Parse should be deleted?
> 
> It's still needed for the initial parse. We could move the initial creation to another point of course, but I do not see the advantage of this.
> 
My point was that the intricacies of FileManager management in ASTUnit are already complicated enough. So it would be really nice if we could get rid of another point where FileManager is getting created.

I'm not really familiar with those tests, so I don't have a clue why they might be failing.


================
Comment at: tools/libclang/CIndexCodeCompletion.cpp:683
   // Parse the resulting source file to find code-completion results.
+  AST->recreateFileManager();
   AllocatedCXCodeCompleteResults *Results = new AllocatedCXCodeCompleteResults(
----------------
It feels like there should be way to do it without changing public API.
Do we really need this call?


https://reviews.llvm.org/D33045