[READY] Automatically load a compilation database if found #680

Merged
merged 1 commit into from Jan 7, 2017

Projects

None yet

7 participants

@puremourning
Collaborator
puremourning commented Jan 1, 2017 edited

Alternative implementation to : #679

See #679 for explanation and rationale.

Fixes: #489
Fixes: Valloric/YouCompleteMe#2310 (sort of)
Fixes: #26
Fixes: Valloric/YouCompleteMe#1981
Fixes: Valloric/YouCompleteMe#1623 (sort of)
Fixes: Valloric/YouCompleteMe#1622
Partly implements: Valloric/YouCompleteMe#927
Fixes: Valloric/YouCompleteMe#664
Fixes: Valloric/YouCompleteMe#487
Fixes (discussion on): Valloric/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 Reviewable

@Valloric
Owner
Valloric commented Jan 1, 2017

@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
Owner
Valloric commented Jan 1, 2017

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
codecov-io commented Jan 1, 2017 edited

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
Collaborator

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
Collaborator
vheon commented Jan 1, 2017

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
Collaborator

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
Collaborator

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

I think this is READY for a proper review cycle.

@puremourning puremourning changed the title from [RFC] Automatically load a compilation database if found: built-in version to [READY] Automatically load a compilation database if found: built-in version Jan 2, 2017
@puremourning puremourning changed the title from [READY] Automatically load a compilation database if found: built-in version to [READY] Automatically load a compilation database if found Jan 2, 2017
@r4nt
r4nt commented Jan 2, 2017

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
Collaborator
puremourning commented Jan 2, 2017 edited

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
Owner
Valloric commented Jan 2, 2017

:lgtm: 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
Collaborator

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
Collaborator
vheon commented Jan 2, 2017

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
Collaborator
micbou commented Jan 3, 2017

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
Collaborator

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
Owner
Valloric commented Jan 3, 2017

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
Owner
Valloric commented Jan 3, 2017

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
Collaborator

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
Collaborator

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
Collaborator

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
Owner
Valloric commented Jan 4, 2017

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
Collaborator
micbou commented Jan 7, 2017

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).

:lgtm:


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 puremourning Load a compilation database by default if found
128b508
@puremourning
Collaborator

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

@puremourning puremourning referenced this pull request in Valloric/YouCompleteMe Jan 7, 2017
Merged

[READY] Update readme for compilation database support #2495

4 of 4 tasks complete
@Valloric
Owner
Valloric commented Jan 7, 2017

Merge it! :)

@puremourning
Collaborator

@homu r=micbou

@homu
Collaborator
homu commented Jan 7, 2017

📌 Commit 128b508 has been approved by micbou

@homu homu added a commit that referenced this pull request Jan 7, 2017
@homu homu Auto merge of #680 - puremourning:compilation-database, r=micbou
[READY] Automatically load a compilation database if found

Alternative implementation to : #679

See #679 for explanation and rationale.

Fixes: #489
Fixes: Valloric/YouCompleteMe#2310 (sort of)
Fixes: #26
Fixes: Valloric/YouCompleteMe#1981
Fixes: Valloric/YouCompleteMe#1623 (sort of)
Fixes: Valloric/YouCompleteMe#1622
Partly implements: Valloric/YouCompleteMe#927
Fixes: Valloric/YouCompleteMe#664
Fixes: Valloric/YouCompleteMe#487
Fixes (discussion on): Valloric/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!)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/680)
<!-- Reviewable:end -->
d19f9c5
@homu
Collaborator
homu commented Jan 7, 2017

⌛️ Testing commit 128b508 with merge d19f9c5...

@homu
Collaborator
homu commented Jan 7, 2017

☀️ Test successful - status

@homu homu merged commit 128b508 into Valloric:master Jan 7, 2017

6 of 7 checks passed

code-review/reviewable Review in progress: 0 of 2 LGTMs obtained (and 2 stale)
Details
codecov/changes No unexpected coverage changes found.
Details
codecov/patch 97.47% of diff hit (target 91.79%)
Details
codecov/project 92.69% (+0.90%) compared to 0e7ad1f
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@homu homu added a commit to Valloric/YouCompleteMe that referenced this pull request Jan 8, 2017
@homu homu Auto merge of #2495 - puremourning:readme-compilation-database, r=micbou
[READY] Update readme for compilation database support

# PR Prelude

Thank you for working on YCM! :)

**Please complete these steps and check these boxes (by putting an `x` inside
the brackets) _before_ filing your PR:**

- [X] I have read and understood YCM's [CONTRIBUTING][cont] document.
- [X] I have read and understood YCM's [CODE_OF_CONDUCT][code] document.
- [X] I have included tests for the changes in my PR. If not, I have included a
  rationale for why I haven't.

> only changes docs

- [X] **I understand my PR may be closed if it becomes obvious I didn't
  actually perform all of these steps.**

# Why this change is necessary and useful

This change:
 - updates the c-family completer documentation to describe the built in support for compilation databases added in Valloric/ycmd#680
 - explains more about why ycmd needs compiler flags, and how to go about providing them
 - recommends using a compilation database (as that seems to be the fashion)
 - standardises formatting for `NOTE` (it was inconsistent before)
 - states that the preferred installation method is `install.py` (rather than the full installation instructions)
 - update the vim doc
 - update the ycmd submodule

### ycmd update release note

- Valloric/ycmd#678 - Bump Boost version to 1.63.0
- Valloric/ycmd#686 - Update JediHTTP for Python 3.6 support
- Valloric/ycmd#684 - Fix JavaScript identifier regex
- Valloric/ycmd#680 - Automatically load a compilation database if found

[cont]: https://github.com/Valloric/YouCompleteMe/blob/master/CONTRIBUTING.md
[code]: https://github.com/Valloric/YouCompleteMe/blob/master/CODE_OF_CONDUCT.md

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2495)
<!-- Reviewable:end -->
d02de4b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment