I left a comment, but this is still .

For what it's worth, my solution to the same problem was a little more complicated. Nice work.

Reviewed 8 of 8 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

cpp/ycm/ClangCompleter/TranslationUnit.cpp, line 592 at r1 (raw file):

  CXFile file = clang_getFile( clang_translation_unit_,
                               location.filename_.c_str() );
  CXSourceLocation source_location = clang_getLocation(

TranslationUnit::GetCursor() and this function share the same algorithm that "returns" a CXSourceLocation from clang_translation_unit_ and a location. Maybe we should move those lines in GetSourceLocation().

Something like:

CXSourceLocation GetSourceLocation( std::string filename, int line, int column ) {
  CXFile file = clang_getFile( clang_translation_unit_, filename.c_str() );
  return clang_getLocation( clang_translation_unit_, file, line, column );
}

Comments from Reviewable

Codecov Report

Merging #1038 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1038      +/-   ##
==========================================
+ Coverage   97.24%   97.29%   +0.04%     
==========================================
  Files          90       90              
  Lines        6981     6987       +6     
==========================================
+ Hits         6789     6798       +9     
+ Misses        192      189       -3

Reviewed 7 of 8 files at r1, 1 of 1 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

cpp/ycm/ClangCompleter/TranslationUnit.cpp, line 592 at r1 (raw file):

Done.

Comments from Reviewable

Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

Bumped the core version.

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.

cpp/ycm/ClangCompleter/TranslationUnit.h, line 124 at r4 (raw file):

  void UpdateLatestDiagnostics();

  // These four methods must be called under the clang_access_mutex_ lock.

Unrelated to this PR, but we should take another look at the clang thread safety annotations.

Comments from Reviewable

with one comment

Reviewed 5 of 8 files at r1, 3 of 3 files at r3, 1 of 1 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

cpp/ycm/ClangCompleter/ClangCompleter.cpp, line 272 at r5 (raw file):

                                                   column,
                                                   unsaved_files,
                                                   reparse ) );

by passing reparse here and later (in GetDocsForLocation) we invoke 2x reparse operations, which could be significant on large TUs. I suppose we can always pass FALSE to GetDocsForLocation because:

• if we created a new TU, it just got parsed
• otherwise, if reparse is true, we did it here, else we don't want to anyway.

Comments from Reviewable

Review status: all files reviewed at latest revision, 1 unresolved discussion.

cpp/ycm/ClangCompleter/ClangCompleter.cpp, line 272 at r5 (raw file):

Done. I added a comment explaining why we never parse a second time.

Comments from Reviewable

Review status: all files reviewed at latest revision, 1 unresolved discussion.

cpp/ycm/ClangCompleter/ClangCompleter.cpp, line 272 at r5 (raw file):

Now that I think about it, are we sure we are parsing the translation unit when we create it? If that's the case then why are we always passing reparse to GetDeclarationLocation?

Comments from Reviewable

Review status: all files reviewed at latest revision, 1 unresolved discussion.

cpp/ycm/ClangCompleter/ClangCompleter.cpp, line 272 at r5 (raw file):

Maybe we need to do this instead:

if ( unit->LocationIsInSystemHeader( location ) ) {
  unit = translation_unit_store_.GetOrCreate( location.filename_,
                                                unsaved_files,
                                                flags );
  return unit->GetDocsForLocation( location, unsaved_files, reparse );
}
return unit->GetDocsForLocation( location, unsaved_files, false );

Comments from Reviewable

Reviewed 1 of 1 files at r7, 1 of 1 files at r8.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

cpp/ycm/ClangCompleter/ClangCompleter.cpp, line 272 at r5 (raw file):

I went for only passing the reparse parameter if the location is in a system header and not reparsing otherwise. The reason is that we can imagine a scenario where the docstring of a system header is modified by an external source while editing code.

Comments from Reviewable

Reviewed 1 of 1 files at r7, 1 of 1 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved.

cpp/ycm/ClangCompleter/ClangCompleter.cpp, line 272 at r5 (raw file):

ok

Comments from Reviewable

Thanks for the review!

@zzbot r++

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable

@zzbot r+

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable

📌 Commit dbc337d has been approved by micbou

⌛ Testing commit dbc337d with merge 1db21a7...

☀️ Test successful - status-appveyor, status-travis
Approved by: micbou
Pushing 1db21a7 to master...