Thanks! This will be very useful.

Reviewed 15 of 15 files at r1.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @puremourning)

README.md, line 105 at r1 (raw file):

for Go (using [Godef][godef] for jumping to definitions), a TSServer-based
completer for JavaScript and TypeScript, a [jdt.ls][jdtls]-based server for
Java, and a [RLS][]-based completer for Rust.  More will be added with time.

Two spaces between "Rust." and "More".

Reviewable status: 1 of 2 LGTMs obtained (waiting on @puremourning)

ycmd/completers/cpp/clangd_completer.py, line 408 at r1 (raw file):

  def _SendFlagsFromExtraConf( self, request_data ):

I believe it make sense to perform _AddMacIncludePaths magic in here as well. WDYT?

ycmd/completers/cpp/clangd_completer.py, line 415 at r1 (raw file):

    with self._server_info_mutex:
      module = extra_conf_store.ModuleForSourceFile( filepath )

This still results in overwrite of local compilation database. Which is unnatural(at least to me) and definitely not consistent with libclang-based compiler. In libclang-based completer lookup order is as follows:

• Use local .ycm_extra_conf.py if exists
• Use local compile_commands.json if exists
• Use global ycm extra conf if exists.

Current approach implementation results in:

• Use local .ycm_extra_conf.py if exists and returns flags
• Use global ycm extra conf if exists and returns flags.
• Use local compile_commands.json if exists

which would both break existing users with local compilation databases and require them to write exceptions in their global ycm extra conf for each project path with a compile_commands.json in them, which is totally useless and against the principal of global ycm extra conf being a fallback.

I believe we should restore the lookup order to be same as libclang-based one.

Also I believe it is better to mimic the logic in libclang-based completer by making use of ycmd.Flags, just by making sure _GetFlagsFromExtraConfOrDatabase returns EMPTY_FLAGS in case of clangd-based completer and loads the compilation database for libclang-based completer. WDYT?

Reviewable status: 1 of 2 LGTMs obtained (waiting on @kadircet, @micbou, and @puremourning)

ycmd/completers/cpp/clangd_completer.py, line 408 at r1 (raw file):

If there's no other way to get clangd to discover macOS system include paths when using extra confs and no database, I don't see what choice we have.

ycmd/completers/cpp/clangd_completer.py, line 415 at r1 (raw file):

History time:

• This proposed order used to be what we did in libclang.
• People complained.
• We made the breaking change and switched to the current libclang order.
• Other people complained.

I know @micbou was against keeping the libclang order, but I think we shouldn't make that change once again.

Reviewable status: 1 of 2 LGTMs obtained (waiting on @kadircet, @micbou, and @puremourning)

ycmd/completers/cpp/clangd_completer.py, line 408 at r1 (raw file):

Sounds like a good plan.

ycmd/completers/cpp/clangd_completer.py, line 415 at r1 (raw file):

perhaps we have to agree to disagree and allow the extra conf settings to return an indicatory to say "use compilation db if present"

Reviewable status: 1 of 2 LGTMs obtained (waiting on @kadircet, @micbou, and @puremourning)

ycmd/completers/cpp/clangd_completer.py, line 415 at r1 (raw file):

I hope you realize that, even with that extra option, ycmd will have an influx of angry users because we changed the behaviour once again and I don't want to deal with them. I also don't think another field in the dictionary won't prevent that influx.

As a user, I do prefer the current libclang order, because otherwise, when I just want the database, I need to reach into my settings and disable the global extra conf.

Note that people who want the global extra conf over a local database can do that with the current libclang ordering (place the global extra conf in $HOME), but if we revert to the old order there's no way to make the local database take priority over the global extra conf, short of completely disabling the global extra conf.

That said, I'll leave the decision to you.

Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic, @kadircet, and @micbou)

README.md, line 105 at r1 (raw file):

Done.

ycmd/completers/cpp/clangd_completer.py, line 415 at r1 (raw file):

I wasn't proposing to change the current behaviour again (that would be the ravings of a madman!), more that we make the 2 behaviours available and allow users to choose, with the default being what we currently do. Shrug.

It turns out that "just using the Flags class" is harder than it looks, but perhaps that's ripe for a refactor anyway.

Reviewable status: 1 of 2 LGTMs obtained (waiting on @kadircet and @micbou)

ycmd/completers/cpp/clangd_completer.py, line 415 at r1 (raw file):
Alright, maybe the "use comp db if present" optional field is a good idea.

It turns out that "just using the Flags class" is harder than it looks, but perhaps that's ripe for a refactor anyway.

You don't need the whole Flags class. Just the _AddMacIncludePaths( flags ) free function.

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic, @kadircet, and @micbou)

ycmd/completers/cpp/clangd_completer.py, line 415 at r1 (raw file):

• used the same logic for extra conf/database/global as Flags
• Added Mac flags and a test. Also I changed the debug info to include the actual thing we send to clangd, which is much more valuable.

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic, @kadircet, and @micbou)

ycmd/completers/cpp/clangd_completer.py, line 415 at r1 (raw file):

Done.

This looks good, but the tests are failing on Windows.

Reviewed 3 of 3 files at r2, 1 of 2 files at r3, 4 of 4 files at r4.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @kadircet, @micbou, and @puremourning)

ycmd/completers/cpp/clangd_completer.py, line 442 at r4 (raw file):

        return

      if ( settings.get( 'do_cache', True ) and

Is there any reason for a user to have do_cache set to False? I don't see any and it is simpler to just assume True.

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic, @kadircet, and @micbou)

ycmd/completers/cpp/clangd_completer.py, line 442 at r4 (raw file):

We do assume true, that's what the second argument to get does (returns True if it's not set or not present)

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @kadircet, @micbou, and @puremourning)

ycmd/completers/cpp/clangd_completer.py, line 442 at r4 (raw file):

That's a different definition of "assume". I was thinking of just skipping the settings.get( 'do_cache', True ) part and ignore whatever the user sets for do_cache.

Strictly speaking, it is a breaking change if a user really wants do_cache to be false, but... why would anyone want that?

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @kadircet and @micbou)

ycmd/completers/cpp/clangd_completer.py, line 442 at r4 (raw file):

I suspect @micbou was trying to obey the ycmd docs for this (unusual) flag. It's not a big deal for me. We don't have a test for it, but we don't have a test for it with the clang completer (probably!) so I say it's harmless to leave it in.

LGTM, thanks!

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic, @micbou, and @puremourning)

ycmd/completers/cpp/clangd_completer.py, line 425 at r6 (raw file):

      module = extra_conf_store.ModuleForSourceFile( filepath )
      if module and not extra_conf_store.IsGlobalExtraConfModule( module ):

nit: maybe simplify a bit with:

# no extra conf, let clangd fallback
if not module:
  return

# global ycm extra conf and local comp db, let clangd use local comp db
if extra_conf_store.IsGlobalExtraConfModule( module ) and CompilationDatabaseExists( filepath ):
  return

# use ycm_extra_conf either local or global
settings = self.GetSettings( module, request_data )

Reviewable status: 1 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic, @micbou, and @puremourning)

Reviewable status: 0 of 2 LGTMs obtained (and 2 stale) (waiting on @bstaletic and @kadircet)

ycmd/completers/cpp/clangd_completer.py, line 425 at r6 (raw file):

Good point. I was trying to mirror the flags.py logic too closely.

Done.

, now that the tests work.
Consider squashing the commits. Or just merge the PR.

Codecov seems to be confused, hopefully its data isn't corrupted and it's just another hiccup.

Reviewed 1 of 6 files at r5, 5 of 5 files at r6, 1 of 1 files at r7, 5 of 5 files at r9.
Reviewable status: 1 of 2 LGTMs obtained (and 1 stale) (waiting on @kadircet)

rebased and squashed.

Reviewed 1 of 5 files at r9, 16 of 16 files at r11.
Reviewable status: 1 of 2 LGTMs obtained (and 1 stale)