[PATCH] D78501: Add a facility to get system cache directory and use it in clangd

[Sam McCall via Phabricator via llvm-commits]

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