sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
Thanks! Looks good with a few nits. Once you're done, let me know if I should land this for you (after a few patches landed this way you can apply for commit access: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access) You can add "Fixes https://github.com/clangd/clangd/issues/341" to the description to close the associated bug. Note to self: maybe we should add this to the release notes so people can delete ~/.clangd. ================ Comment at: llvm/include/llvm/Support/Path.h:372 +/// Get the directory where installed packages should put their +/// machine-local cache. +/// ---------------- I'd add "e.g. $XDG_CACHE_HOME" to this comment. (No need to spell out the fallback or windows behavior, but this gives a bit more of a hint) ================ Comment at: llvm/include/llvm/Support/Path.h:375 +/// @param result Holds the resulting path name. +/// @result True if the appropriate directory was found. +bool cache_directory(SmallVectorImpl<char> &result); ---------------- This kind of implies you check that it exists (which it may not, particularly on the ~/.cache fallback). Avoiding IO seems like the right thing, but I'd mention in the function description that the returned path may not exist yet. ================ Comment at: llvm/lib/Support/Unix/Path.inc:1142 +bool cache_directory(SmallVectorImpl<char> &result) { + char *RequestedDir = getenv("XDG_CACHE_HOME"); + if (RequestedDir) { ---------------- nit: suggest const char* to signal we're not doing anything bad with this terrifying API :-) ================ Comment at: llvm/lib/Support/Unix/Path.inc:1143 + char *RequestedDir = getenv("XDG_CACHE_HOME"); + if (RequestedDir) { + result.clear(); ---------------- nit: llvm style is to inline this `if (char * RequestedDir = ...)` ================ Comment at: llvm/unittests/Support/Path.cpp:363 + + std::string expected; + ---------------- nit: Expected (uppercase for vars in llvm style. The style in Path.h/.inc is very old/mixed up and you've done a great job being consistent, but at least for local vars in the cpp file we should follow the guidelines) ================ Comment at: llvm/unittests/Support/Path.cpp:426 +#ifdef _WIN32 + std::wstring OriginalStorage; + if (wchar_t const *path = ::_wgetenv(L"XDG_CACHE_HOME")) { ---------------- nit: use an llvm::Optional<wstring> so we correctly distinguish unset vs empty?, and elsewhere (I guess we should extract a RAII environment-restorer for this file, but not going to ask you to boil that ocean :-)) ================ Comment at: llvm/unittests/Support/Path.cpp:464 + SmallString<128> HomeDir; + if (path::home_directory(HomeDir)) { + path::append(HomeDir, ".cache"); ---------------- you can just ASSERT_TRUE, this should always succeed in our test setups I think. ================ Comment at: llvm/unittests/Support/Path.cpp:469 + + if (!expected.empty()) { + SmallString<128> CacheDir; ---------------- Why are we testing this twice, once conditionally? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78501/new/ https://reviews.llvm.org/D78501 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits