[READY] Automatically load a compilation database if found

[puremourning]

Alternative implementation to : #679

See #679 for explanation and rationale.

Fixes: #489
Fixes: ycm-core/YouCompleteMe#2310 (sort of)
Fixes: #26
Fixes: ycm-core/YouCompleteMe#1981
Fixes: ycm-core/YouCompleteMe#1623 (sort of)
Fixes: ycm-core/YouCompleteMe#1622
Partly implements: ycm-core/YouCompleteMe#927
Fixes: ycm-core/YouCompleteMe#664
Fixes: ycm-core/YouCompleteMe#487
Fixes (discussion on): ycm-core/YouCompleteMe#174

This implementation is almost identical to the default-extra-conf version, except:

• the code lives in flags.py
• it re-uses INCLUDE_FLAGS from flags.py
• it provides the directory of the compilation database in the clang_completer debug info instead of the name of the default extra conf module.

On reflection, I think I might prefer this implementation.

For now, the tests are identical to the other PR (you can now see why I added that set of tests!)

---

This change is 

[Valloric]

@r4nt I remember you wanting to see this implemented in ycmd. Curious what your thoughts are; check out both this PR and the other implementation (linked in PR description).

[Valloric]

Gave a light review pass. I'm leaning towards this approach as well now that I've seen both. The other approach is a default extra conf file, but the we load it by default and the user would be unwise to change that file directly... at that point we might as well just have this default logic hard-coded into ycmd, because it effectively is.

As usual, thanks for the PR! :)

---

Review status: 0 of 6 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

cpp/ycm/ClangCompleter/CompilationDatabase.h, line 57 at r1 (raw file):

    const boost::python::object &path_to_file );

  const char * GetDatabaseDirectory()

I'm pretty sure you can just make this return an std::string and Boost.Python will know what to do with it.

Let's avoid returning raw pointers if possible.

---

ycmd/completers/cpp/flags.py, line 96 at r1 (raw file):

    # We cache the database for any given source directory
    self.compilation_database_dir_map = dict()
    # Sometimes we don't actually know what the flags to use are. Rather than

Empty line before comment block?

---

ycmd/completers/cpp/flags.py, line 232 at r1 (raw file):

  # database already for that path, or if a compile_commands.json file exists in
  # that directory.
  def FindCompilationDatabase( self, wd ):

Let's expand wd to a longer name.

---

ycmd/completers/cpp/flags.py, line 235 at r1 (raw file):

    # Find all the ancestor directories of the supplied directory
    for folder in PathsToAllParentFolders( wd ):
      # Did we already cache a database for this path?

Maybe cut down on some of these obvious comments now that this isn't in a user-facing file. :)

---

Comments from Reviewable

[codecov-io]

Current coverage is 92.69% (diff: 97.47%)

Merging #680 into master will increase coverage by 0.90%

@@             master       #680   diff @@
==========================================
  Files            79         79          
  Lines          5166       5258    +92   
  Methods         295        297     +2   
  Messages          0          0          
  Branches        139        139          
==========================================
+ Hits           4742       4874   +132   
+ Misses          368        329    -39   
+ Partials         56         55     -1   

Powered by Codecov. Last update 0e7ad1f...128b508

[puremourning]

Review status: 0 of 6 files reviewed at latest revision, 4 unresolved discussions.

---

cpp/ycm/ClangCompleter/CompilationDatabase.h, line 57 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

I'm pretty sure you can just make this return an std::string and Boost.Python will know what to do with it.

Let's avoid returning raw pointers if possible.

Old habit. Trying to avoid a string copy (std::string& and const std::string& don't work). But truth is I didn't check whether the underlying implementation doesn't copy the string anyway, so meh, changed to std::string return.

---

ycmd/completers/cpp/flags.py, line 96 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

Empty line before comment block?

Done.

---

ycmd/completers/cpp/flags.py, line 232 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

Let's expand wd to a longer name.

Done.

---

ycmd/completers/cpp/flags.py, line 235 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

Maybe cut down on some of these obvious comments now that this isn't in a user-facing file. :)

Yep, for this revision i literally copy/pasted the whole code block :)

---

Comments from Reviewable

[vheon]

Review status: 0 of 6 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

ycmd/completers/cpp/clang_completer.py, line 37 at r3 (raw file):

from ycmd.completers.completer import Completer
from ycmd.completers.completer_utils import GetIncludeStatementValue
from ycmd.completers.cpp.flags import ( Flags, PrepareFlagsForClang )

why the parenthesis here?

---

Comments from Reviewable

[puremourning]

Review status: 0 of 6 files reviewed at latest revision, 5 unresolved discussions.

---

ycmd/completers/cpp/clang_completer.py, line 37 at r3 (raw file):

Previously, vheon (Andrea Cedraro) wrote…

why the parenthesis here?

I think because i changed this then changed it back. Fixed.

---

Comments from Reviewable

[puremourning]

Tests are passing, we're happy with this approach, there's some draft documentation.

I think this is READY for a proper review cycle.

[r4nt]

Yep, agree that built-in is probably better. Very happy to see projects that use the compilation db work just out of the box. Thanks so much for this.
If I had one wish, it would be longest common suffix matching for .h/.cpp paths, but that might be too brittle :)

[puremourning]

If I had one wish, it would be longest common suffix matching for .h/.cpp paths, but that might be too brittle

Interesting. Thanks! We were chatting about this last night. Our plan is go out something simple live then improve the heuristics based on feedback and/or inspiration. We considered more caches and some path manipulations for header files (the latter being the key goal).

Do you have experience with longest common suffix? Do you know that it works well? E.g. does it work for clang code base? If so, that could be our first feedback/inspiration point. :)

[Valloric]

Nice stuff! Some minor inline comments, otherwise great.

---

Review status: 0 of 8 files reviewed at latest revision, 4 unresolved discussions.

---

ycmd/completers/cpp/flags.py, line 97 at r4 (raw file):

    # We cache the compilation database for any given source directory
    self.compilation_database_dir_map = dict()

Since this is python, I'd like to see docs stating what the type of key and value are.

---

ycmd/completers/cpp/flags.py, line 104 at r4 (raw file):

    # percentage of cases and allow new files (which are not yet in the
    # compilation database) to receive at least some flags
    self.file_directory_heuristic_map = dict()

Same as above; I'd like to see docs stating what the type of key and value are.

God, I hate dynamic languages...

---

ycmd/tests/clang/init.py, line 104 at r4 (raw file):

@contextlib.contextmanager
def TemporaryProject( tmp_dir, compile_commands ):

"Project" seems to be used here to imply a compile_command.json connection. That's not very obvious to me. Maybe "ClangProject"? Something better than "project" which is too ambiguous.

---

Comments from Reviewable

[puremourning]

Thanks for the review!

---

Review status: 0 of 8 files reviewed at latest revision, 5 unresolved discussions.

---

ycmd/completers/cpp/flags.py, line 97 at r4 (raw file):

Previously, Valloric (Val Markovic) wrote…

Since this is python, I'd like to see docs stating what the type of key and value are.

Done.

---

ycmd/completers/cpp/flags.py, line 104 at r4 (raw file):

Previously, Valloric (Val Markovic) wrote…

Same as above; I'd like to see docs stating what the type of key and value are.

God, I hate dynamic languages...

Done.

---

ycmd/tests/clang/init.py, line 104 at r4 (raw file):

Previously, Valloric (Val Markovic) wrote…

"Project" seems to be used here to imply a compile_command.json connection. That's not very obvious to me. Maybe "ClangProject"? Something better than "project" which is too ambiguous.

Done. ClangProject seems ok.

---

Comments from Reviewable

[vheon]

Reviewed 2 of 6 files at r1, 1 of 3 files at r2, 1 of 5 files at r4, 1 of 5 files at r5.
Review status: 4 of 8 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

ycmd/completers/cpp/clang_completer.py, line 388 at r4 (raw file):

      database = self._flags.FindCompilationDatabase(
          os.path.dirname( filename ) )
      if database is None or not database.DatabaseSuccessfullyLoaded():

can't we write this as if not database?

---

ycmd/completers/cpp/flags.py, line 104 at r5 (raw file):

    # Sometimes we don't actually know what the flags to use are. Rather than
    # returning no flags, if we've previously found flags for a file in a
    # particular directory, return them. The will probably work in a high

Just a nitpick but is "The" right here?

---

ycmd/completers/cpp/flags.py, line 198 at r5 (raw file):

    database = self.FindCompilationDatabase( file_dir )
    if database is None or not database.DatabaseSuccessfullyLoaded():

We call this function in two places and in both places we do this check. Couldn't we raise directly in FindCompilationDatabase and adapt the other call to try: catch this? I'm not sure that is better but since this check is done twice I leaning to think that it deserves at least a name (even though is a simple condition)?

---

ycmd/completers/cpp/flags.py, line 205 at r5 (raw file):

                                                   file_extension )

    if compilation_info is None:

I think is more Pythonic to write it as if not compilation_info?

---

Comments from Reviewable

[micbou]

Reviewed 3 of 6 files at r1, 3 of 3 files at r2, 2 of 2 files at r3, 5 of 5 files at r4, 4 of 5 files at r5.
Review status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

---

cpp/ycm/ClangCompleter/CompilationDatabase.h, line 58 at r2 (raw file):

  std::string GetDatabaseDirectory()
  {

Isn't the open bracket supposed to be on the first line according to our coding style?

---

ycmd/completers/cpp/clang_completer.py, line 402 at r2 (raw file):

                 '  No configuration file found\n'
                 '  No compilation database file found\n' )
      else:

This else: is not needed since we are returning in the first branch.

---

ycmd/completers/cpp/clang_completer.py, line 391 at r4 (raw file):

        return ( 'C-family completer debug information:\n'
                 '  No configuration file found\n'
                 '  No compilation database found\n' )

Last \n should be removed.

---

ycmd/completers/cpp/clang_completer.py, line 397 at r5 (raw file):

                 '  Using compilation database from: {0}\n'
                 '  Flags: {1}'.format( database.database_directory,
                                        list( flags ) ) )

It's not obvious why flagsis always defined in this case and the one below. I think we should move the flags assignment in its own try/catch block like this:

try:
  extra_conf = extra_conf_store.ModuleFileForSourceFile( filename )
except UnknownExtraConf as error:
  return ...

try:
  flags = self._FlagsForRequest( request_data ) or []
except NoExtraConfDetected:
  return ...

database = self._flags.compilation_database_dir_map.get( 
    os.path.dirname( filename ) )

if database:
  return ... flags ...
return ... flags ...

This way, it is clear that flags is always defined.

---

ycmd/completers/cpp/flags.py, line 186 at r2 (raw file):

  def Clear( self ):
    self.flags_for_file.clear()

I think this function should also clear the compilation_database_dir_map and file_directory_heuristic_map dictionaries.

---

ycmd/completers/cpp/flags.py, line 191 at r2 (raw file):

  def _GetFlagsFromCompilationDatabase( self, file_name ):
    file_dir = os.path.dirname( file_name )
    ( file_root, file_extension ) = os.path.splitext( file_name )

Parentheses are not required.

---

ycmd/completers/cpp/flags.py, line 248 at r5 (raw file):

    # Nothing was found. No compilation flags are available.
    # Note: we cache the fact that none was found for this folder to speed up
    # subsequent searches..

One dot can be removed.

---

ycmd/tests/clang/init.py, line 133 at r4 (raw file):

    yield
  finally:
    os.unlink( path )

We use os.remove in other places so we should also use it here to be consistent (os.remove and os.unlink are identicals).

---

ycmd/tests/clang/flags_test.py, line 513 at r2 (raw file):

def _MakeRelativePathsInFlagsAvsoluteTest( test ):

Typo: Avsolute instead of Absolute.

---

ycmd/tests/clang/flags_test.py, line 513 at r4 (raw file):

def CompilationDatabase_MakeRelativePathsInFlagsAbsolute_test():

This test is not directly related to compilation databases so I would not prefix its name with CompilationDatabase. Same comment for the tests below.

---

Comments from Reviewable

[puremourning]

Review status: 3 of 8 files reviewed at latest revision, 17 unresolved discussions.

---

cpp/ycm/ClangCompleter/CompilationDatabase.h, line 58 at r2 (raw file):

Previously, micbou wrote…

Isn't the open bracket supposed to be on the first line according to our coding style?

it is indeed.

---

ycmd/completers/cpp/clang_completer.py, line 402 at r2 (raw file):

Previously, micbou wrote…

This else: is not needed since we are returning in the first branch.

Done.

Removed by refactor of exceptions.

---

ycmd/completers/cpp/clang_completer.py, line 388 at r4 (raw file):

Previously, vheon (Andrea Cedraro) wrote…

can't we write this as if not database?

yeah, i just hate dynamic languages. None behaves strangely.

---

ycmd/completers/cpp/clang_completer.py, line 391 at r4 (raw file):

Previously, micbou wrote…

Last \n should be removed.

Done.

---

ycmd/completers/cpp/clang_completer.py, line 397 at r5 (raw file):

Previously, micbou wrote…

It's not obvious why flagsis always defined in this case and the one below. I think we should move the flags assignment in its own try/catch block like this:

try:
  extra_conf = extra_conf_store.ModuleFileForSourceFile( filename )
except UnknownExtraConf as error:
  return ...

try:
  flags = self._FlagsForRequest( request_data ) or []
except NoExtraConfDetected:
  return ...

database = self._flags.compilation_database_dir_map.get( 
    os.path.dirname( filename ) )

if database:
  return ... flags ...
return ... flags ...

This way, it is clear that flags is always defined.

I've changed the control flow here to make it more obvious and added a few comments.

---

ycmd/completers/cpp/flags.py, line 186 at r2 (raw file):

Previously, micbou wrote…

I think this function should also clear the compilation_database_dir_map and file_directory_heuristic_map dictionaries.

Done.

---

ycmd/completers/cpp/flags.py, line 191 at r2 (raw file):

Previously, micbou wrote…

Parentheses are not required.

Done.

---

ycmd/completers/cpp/flags.py, line 104 at r5 (raw file):

Previously, vheon (Andrea Cedraro) wrote…

Just a nitpick but is "The" right here?

Done.

---

ycmd/completers/cpp/flags.py, line 198 at r5 (raw file):

Previously, vheon (Andrea Cedraro) wrote…

We call this function in two places and in both places we do this check. Couldn't we raise directly in FindCompilationDatabase and adapt the other call to try: catch this? I'm not sure that is better but since this check is done twice I leaning to think that it deserves at least a name (even though is a simple condition)?

Yeah, I have it this way in another change. It is better to have FindCompilatoinDatabase raise NoCompilationDatabase This required a bit of refactoring.

---

ycmd/completers/cpp/flags.py, line 205 at r5 (raw file):

Previously, vheon (Andrea Cedraro) wrote…

I think is more Pythonic to write it as if not compilation_info?

grr. done :)

---

ycmd/completers/cpp/flags.py, line 248 at r5 (raw file):

Previously, micbou wrote…

One dot can be removed.

Done.

---

Comments from Reviewable

[Valloric]

Review status: 3 of 8 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

ycmd/completers/cpp/flags.py, line 97 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Done.

Maybe use "key" instead of "LHS" and "value" instead of "RHS"? Seems more obvious.

---

ycmd/completers/cpp/flags.py, line 104 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Done.

Same as above. RHS is a needless abbreviation, just use "value".

---

Comments from Reviewable

[Valloric]

Reviewed 2 of 6 files at r1, 1 of 5 files at r5, 5 of 5 files at r6.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[puremourning]

Review status: 7 of 8 files reviewed at latest revision, 9 unresolved discussions.

---

ycmd/tests/clang/init.py, line 133 at r4 (raw file):

Previously, micbou wrote…

We use os.remove in other places so we should also use it here to be consistent (os.remove and os.unlink are identicals).

Done.

---

ycmd/tests/clang/flags_test.py, line 513 at r2 (raw file):

Previously, micbou wrote…

Typo: Avsolute instead of Absolute.

Done.

---

ycmd/tests/clang/flags_test.py, line 513 at r4 (raw file):

Previously, micbou wrote…

This test is not directly related to compilation databases so I would not prefix its name with CompilationDatabase. Same comment for the tests below.

Done.

---

Comments from Reviewable

[puremourning]

Review status: 6 of 8 files reviewed at latest revision, 6 unresolved discussions.

---

ycmd/completers/cpp/flags.py, line 97 at r4 (raw file):

Previously, Valloric (Val Markovic) wrote…

Maybe use "key" instead of "LHS" and "value" instead of "RHS"? Seems more obvious.

Done

---

Comments from Reviewable

[puremourning]

Review status: 6 of 8 files reviewed at latest revision, 6 unresolved discussions.

---

ycmd/completers/cpp/flags.py, line 104 at r4 (raw file):

Previously, Valloric (Val Markovic) wrote…

Same as above. RHS is a needless abbreviation, just use "value".

Done.

---

Comments from Reviewable

[Valloric]

Reviewed 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

ycmd/tests/clang/debug_info_test.py, line 98 at r5 (raw file):

                        '  No configuration file found\n'
                        '  Using compilation database from: {0}\n'
                        '  Flags: .+-Wall.+'.format( tmp_dir ) ) )

Oh nice!

---

Comments from Reviewable

[micbou]

I tested it on our code base and, to my surprise, it worked great. I only needed to copy the compile_commands.json file into the root project. We could even drop our .ycm_extra_conf.py file with this PR (or maybe not).

---

Reviewed 5 of 5 files at r6, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[puremourning]

I've rebased and just tweaked a few comments which were wrong or contained typos.

Is this ready to merge now? I was thinking we could merge this, merge my documentation updates to YCM and update ycmd submodule. This would release python 3.6 support, a few bug fixes and this change. Major release! :)

---

Review status: 7 of 8 files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[Valloric]

Merge it! :)

[puremourning]

@homu r=micbou

[homu]

📌 Commit 128b508 has been approved by micbou

[homu]

⌛ Testing commit 128b508 with merge d19f9c5...

[homu]

☀️ Test successful - status