[clang-tools-extra] r367112 - [clangd] Fix background index not triggering on windows due to case mismatch.
Hans Wennborg via cfe-commits
cfe-commits at lists.llvm.org
Fri Jul 26 10:41:29 PDT 2019
Merged to release_90 in r367135.
On Fri, Jul 26, 2019 at 7:06 AM Sam McCall via cfe-commits
<cfe-commits at lists.llvm.org> wrote:
>
> Author: sammccall
> Date: Fri Jul 26 07:07:11 2019
> New Revision: 367112
>
> URL: http://llvm.org/viewvc/llvm-project?rev=367112&view=rev
> Log:
> [clangd] Fix background index not triggering on windows due to case mismatch.
>
> Summary:
> This isn't a general fix to all paths where we assume case-sensitivity, it's
> a minimally-invasive fix targeting the llvm 9 branch.
>
> Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits
>
> Tags: #clang
>
> Differential Revision: https://reviews.llvm.org/D65320
>
> Modified:
> clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp
> clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h
>
> Modified: clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp?rev=367112&r1=367111&r2=367112&view=diff
> ==============================================================================
> --- clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp (original)
> +++ clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp Fri Jul 26 07:07:11 2019
> @@ -117,20 +117,41 @@ DirectoryBasedGlobalCompilationDatabase:
> return None;
> }
>
> -std::pair<tooling::CompilationDatabase *, /*SentBroadcast*/ bool>
> +// For platforms where paths are case-insensitive (but case-preserving),
> +// we need to do case-insensitive comparisons and use lowercase keys.
> +// FIXME: Make Path a real class with desired semantics instead.
> +// This class is not the only place this problem exists.
> +// FIXME: Mac filesystems default to case-insensitive, but may be sensitive.
> +
> +static std::string maybeCaseFoldPath(PathRef Path) {
> +#if defined(_WIN32) || defined(__APPLE__)
> + return Path.lower();
> +#else
> + return Path;
> +#endif
> +}
> +
> +static bool pathEqual(PathRef A, PathRef B) {
> +#if defined(_WIN32) || defined(__APPLE__)
> + return A.equals_lower(B);
> +#else
> + return A == B;
> +#endif
> +}
> +
> +DirectoryBasedGlobalCompilationDatabase::CachedCDB &
> DirectoryBasedGlobalCompilationDatabase::getCDBInDirLocked(PathRef Dir) const {
> // FIXME(ibiryukov): Invalidate cached compilation databases on changes
> - auto CachedIt = CompilationDatabases.find(Dir);
> - if (CachedIt != CompilationDatabases.end())
> - return {CachedIt->second.CDB.get(), CachedIt->second.SentBroadcast};
> - std::string Error = "";
> -
> - CachedCDB Entry;
> - Entry.CDB = tooling::CompilationDatabase::loadFromDirectory(Dir, Error);
> - auto Result = Entry.CDB.get();
> - CompilationDatabases[Dir] = std::move(Entry);
> -
> - return {Result, false};
> + // FIXME(sammccall): this function hot, avoid copying key when hitting cache.
> + auto Key = maybeCaseFoldPath(Dir);
> + auto R = CompilationDatabases.try_emplace(Key);
> + if (R.second) { // Cache miss, try to load CDB.
> + CachedCDB &Entry = R.first->second;
> + std::string Error = "";
> + Entry.CDB = tooling::CompilationDatabase::loadFromDirectory(Dir, Error);
> + Entry.Path = Dir;
> + }
> + return R.first->second;
> }
>
> llvm::Optional<DirectoryBasedGlobalCompilationDatabase::CDBLookupResult>
> @@ -139,38 +160,41 @@ DirectoryBasedGlobalCompilationDatabase:
> assert(llvm::sys::path::is_absolute(Request.FileName) &&
> "path must be absolute");
>
> + bool ShouldBroadcast = false;
> CDBLookupResult Result;
> - bool SentBroadcast = false;
>
> {
> std::lock_guard<std::mutex> Lock(Mutex);
> + CachedCDB *Entry = nullptr;
> if (CompileCommandsDir) {
> - std::tie(Result.CDB, SentBroadcast) =
> - getCDBInDirLocked(*CompileCommandsDir);
> - Result.PI.SourceRoot = *CompileCommandsDir;
> + Entry = &getCDBInDirLocked(*CompileCommandsDir);
> } else {
> // Traverse the canonical version to prevent false positives. i.e.:
> // src/build/../a.cc can detect a CDB in /src/build if not canonicalized.
> + // FIXME(sammccall): this loop is hot, use a union-find-like structure.
> actOnAllParentDirectories(removeDots(Request.FileName),
> - [this, &SentBroadcast, &Result](PathRef Path) {
> - std::tie(Result.CDB, SentBroadcast) =
> - getCDBInDirLocked(Path);
> - Result.PI.SourceRoot = Path;
> - return Result.CDB != nullptr;
> + [&](PathRef Path) {
> + Entry = &getCDBInDirLocked(Path);
> + return Entry->CDB != nullptr;
> });
> }
>
> - if (!Result.CDB)
> + if (!Entry || !Entry->CDB)
> return llvm::None;
>
> // Mark CDB as broadcasted to make sure discovery is performed once.
> - if (Request.ShouldBroadcast && !SentBroadcast)
> - CompilationDatabases[Result.PI.SourceRoot].SentBroadcast = true;
> + if (Request.ShouldBroadcast && !Entry->SentBroadcast) {
> + Entry->SentBroadcast = true;
> + ShouldBroadcast = true;
> + }
> +
> + Result.CDB = Entry->CDB.get();
> + Result.PI.SourceRoot = Entry->Path;
> }
>
> // FIXME: Maybe make the following part async, since this can block retrieval
> // of compile commands.
> - if (Request.ShouldBroadcast && !SentBroadcast)
> + if (ShouldBroadcast)
> broadcastCDB(Result);
> return Result;
> }
> @@ -200,9 +224,9 @@ void DirectoryBasedGlobalCompilationData
> if (!It.second)
> return true;
>
> - auto Res = getCDBInDirLocked(Path);
> - It.first->second = Res.first != nullptr;
> - return Path == Result.PI.SourceRoot;
> + CachedCDB &Entry = getCDBInDirLocked(Path);
> + It.first->second = Entry.CDB != nullptr;
> + return pathEqual(Path, Result.PI.SourceRoot);
> });
> }
> }
> @@ -213,7 +237,7 @@ void DirectoryBasedGlobalCompilationData
> // Independent of whether it has an entry for that file or not.
> actOnAllParentDirectories(File, [&](PathRef Path) {
> if (DirectoryHasCDB.lookup(Path)) {
> - if (Path == Result.PI.SourceRoot)
> + if (pathEqual(Path, Result.PI.SourceRoot))
> // Make sure listeners always get a canonical path for the file.
> GovernedFiles.push_back(removeDots(File));
> // Stop as soon as we hit a CDB.
>
> Modified: clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h?rev=367112&r1=367111&r2=367112&view=diff
> ==============================================================================
> --- clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h (original)
> +++ clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h Fri Jul 26 07:07:11 2019
> @@ -80,8 +80,13 @@ public:
> llvm::Optional<ProjectInfo> getProjectInfo(PathRef File) const override;
>
> private:
> - std::pair<tooling::CompilationDatabase *, /*SentBroadcast*/ bool>
> - getCDBInDirLocked(PathRef File) const;
> + /// Caches compilation databases loaded from directories.
> + struct CachedCDB {
> + std::string Path; // Not case-folded.
> + std::unique_ptr<clang::tooling::CompilationDatabase> CDB = nullptr;
> + bool SentBroadcast = false;
> + };
> + CachedCDB &getCDBInDirLocked(PathRef File) const;
>
> struct CDBLookupRequest {
> PathRef FileName;
> @@ -98,12 +103,7 @@ private:
> void broadcastCDB(CDBLookupResult Res) const;
>
> mutable std::mutex Mutex;
> - /// Caches compilation databases loaded from directories(keys are
> - /// directories).
> - struct CachedCDB {
> - std::unique_ptr<clang::tooling::CompilationDatabase> CDB = nullptr;
> - bool SentBroadcast = false;
> - };
> + // Keyed by possibly-case-folded directory path.
> mutable llvm::StringMap<CachedCDB> CompilationDatabases;
>
> /// Used for command argument pointing to folder where compile_commands.json
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list