[PATCH] D51725: Allow un-setting the compilation database path

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a subscriber: sammccall.
ilya-biryukov added a comment.

In https://reviews.llvm.org/D51725#1255695, @simark wrote:

> But I am wondering what to do with the feature that allows clangd to change compilation database path using the `didChangeConfiguration` notification.  In my opinion, it's a bug that it's not possible to switch back to use no explicit compilation database path, so I'd still like to get this patch merged.  Or, if we decide this is really not useful, then it should be removed.  I don't think we should keep the feature in the current state, either we fix it or remove it.


Fair point. The original patch looked scary mostly because of `Optional<Optional<...>>`. Many thanks for clearing up your use-case, though. I thought Theia was exclusively using custom directories for compile_commands.json, I didn't know the user had an option to turn this behavior on/off and configure directories explicitly.
I would question the usefulness of the option if Theia can live without it, so I'd vouch for removing it. @sammccall, WDYT? Especially wrt to auto-indexing and it being a customizable build directory?

> Even if right now it's not very expensive to restart clangd, do you foresee a situation in the future where it could become more expensive?

We should keep clangd restarts fast, I don't see any reason why they should be slow.
Changing compile_commands.json files would probably always require both reparsing all the files and reloading all the indexes, so there's little hope we can make clangd faster in those cases, unless we're willing to keep multiple versions of the AST/indexes in memory.

> Also, I tried to remove the double Optional and do as you suggested (use the empty string to mean to use no explicit compilation database path), and in the end I find it more confusing, because we need special cases at both ends.  When we deserialize the JSON and the value is null, and when storing the value in DirectoryBasedGlobalCompilationDatabase, where we convert an empty string to an empty Optional.

Yeah, moving this special meaning into all layers was a bad idea.
Maybe replace an empty optional with an empty string on deseraizliation? The rest of the code using `Optional<Path>` seems fine, it's just the `ClangdConfigurationParamsChange` definition that looks complicated.
The code for deserializing would look like this:

  bool fromJSON(const json::Value &Params, ClangdConfigurationParamsChange &CCPC) {
    json::ObjectMapper O(Params);
    if (!O)
      return ...;
  
    if (const Value* CDBPath = Params->get("compilationDatabasePath")) {
      // Have to update the DB path, CCPC.compilationDatabasePath will have a value.
      llvm::Optional<std::string> NewDBPath;
      if (!fromJSON(CDBPath, NewDBPath))
        return false;
      CCPC.compilationDatabasePath = NewDBPath ? std::move(NewDBPath) : "";
    } else {
      // No need to update DB path, CCPC.compilationDatabasePath will be llvm::None.
    }
    ...
  }


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51725