arphaman added inline comments.
================ Comment at: lib/Frontend/ASTUnit.cpp:1152 + else + SrcLocCache.clear(); ---------------- yvvan wrote: > arphaman wrote: > > Why is `clear` in an `else` here? We always create a new `SourceManager` in > > this function, so the previously cached locations will be invalid, so > > shouldn't we always clear the cache before `TranslateStoredDiagnostics`? > When we load diagnostics that means that preamble has not changed. Doesn't > that mean that source locations can be reused? What can cause them to become > invalid? > When preamble is invalidated - cache is cleared. > > I can keep cache only during TranslateStoredDiagnostics calls but in that > case performance improvement is way less. > > But if you say that current solution is invalid I will do that You're right actually, we can reuse them. We already do make that assumption that the preamble's source locations can be reused in `checkAndSanitizeDiags`. This code is fine then, sorry about the confusion. You should mention in the comment for the `SrcLocCache` that we cache only the source locations from the preamble as we can guarantee that they will stay valid when the source manager is re-created. https://reviews.llvm.org/D33493 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
