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

[Sam McCall via cfe-commits]

On Wed, Apr 29, 2020 at 12:52 AM Eric Christopher 
wrote:

> I've had to temporarily revert this as it was breaking some tests:
>
> .../sources/llvm-project/llvm/unittests/Support/Path.cpp:334:5: error:
> use of undeclared identifier 'set'
> set(Value);
> ^
> 1 error generated.
>
> Happy to help diagnose this with you if you need some assistance.
>
Doh, thanks for the revert.
I made one last change... and ran the wrong tests :-(
Relanded as 4e769e93b90b4d83ee795cda51bea9f76ca5ad5b


>
> Thanks!
>
> -eric
>
> On Tue, Apr 28, 2020 at 3:07 PM Vojtěch Štěpančík via Phabricator via
> llvm-commits  wrote:
>
>> VojtechStep added a comment.
>>
>> Great, thanks for the fixes!
>>
>>
>> Repository:
>>   rG LLVM Github Monorepo
>>
>> CHANGES SINCE LAST ACTION
>>   https://reviews.llvm.org/D78501/new/
>>
>> https://reviews.llvm.org/D78501
>>
>>
>>
>> ___
>> llvm-commits mailing list
>> llvm-comm...@lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eric Christopher via cfe-commits]

I've had to temporarily revert this as it was breaking some tests:

.../sources/llvm-project/llvm/unittests/Support/Path.cpp:334:5: error:
use of undeclared identifier 'set'
set(Value);
^
1 error generated.

Happy to help diagnose this with you if you need some assistance.

Thanks!

-eric

On Tue, Apr 28, 2020 at 3:07 PM Vojtěch Štěpančík via Phabricator via
llvm-commits  wrote:

> VojtechStep added a comment.
>
> Great, thanks for the fixes!
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D78501/new/
>
> https://reviews.llvm.org/D78501
>
>
>
> ___
> llvm-commits mailing list
> llvm-comm...@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Vojtěch Štěpančík via Phabricator via cfe-commits]

VojtechStep added a comment.

Great, thanks for the fixes!


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

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

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGad38f4b371bd: Add a facility to get system cache directory 
and use it in clangd (authored by VojtechStep, committed by sammccall).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78501/new/

https://reviews.llvm.org/D78501

Files:
  clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp
  llvm/include/llvm/Support/Path.h
  llvm/lib/Support/Unix/Path.inc
  llvm/lib/Support/Windows/Path.inc
  llvm/unittests/Support/Path.cpp

Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -13,6 +13,7 @@
 #include "llvm/ADT/Triple.h"
 #include "llvm/BinaryFormat/Magic.h"
 #include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Compiler.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/ErrorHandling.h"
@@ -305,15 +306,46 @@
   }
 }
 
-TEST(Support, HomeDirectory) {
-  std::string expected;
 #ifdef _WIN32
-  if (wchar_t const *path = ::_wgetenv(L"USERPROFILE")) {
+std::string getEnvWin(const wchar_t *Var) {
+  std::string expected;
+  if (wchar_t const *path = ::_wgetenv(Var)) {
 auto pathLen = ::wcslen(path);
 ArrayRef ref{reinterpret_cast(path),
pathLen * sizeof(wchar_t)};
 convertUTF16ToUTF8String(ref, expected);
   }
+  return expected;
+}
+#else
+// RAII helper to set and restore an environment variable.
+class WithEnv {
+  const char *Var;
+  llvm::Optional OriginalValue;
+
+public:
+  WithEnv(const char *Var, const char *Value) : Var(Var) {
+if (const char *V = ::getenv(Var))
+  OriginalValue.emplace(V);
+if (Value)
+  ::setenv(Var, Value, 1);
+else
+  ::unsetenv(Var);
+set(Value);
+  }
+  ~WithEnv() {
+if (OriginalValue)
+  ::setenv(Var, OriginalValue->c_str(), 1);
+else
+  ::unsetenv(Var);
+  }
+};
+#endif
+
+TEST(Support, HomeDirectory) {
+  std::string expected;
+#ifdef _WIN32
+  expected = getEnvWin(L"USERPROFILE");
 #else
   if (char const *path = ::getenv("HOME"))
 expected = path;
@@ -330,31 +362,48 @@
 
 #ifdef LLVM_ON_UNIX
 TEST(Support, HomeDirectoryWithNoEnv) {
-  std::string OriginalStorage;
-  char const *OriginalEnv = ::getenv("HOME");
-  if (OriginalEnv) {
-// We're going to unset it, so make a copy and save a pointer to the copy
-// so that we can reset it at the end of the test.
-OriginalStorage = OriginalEnv;
-OriginalEnv = OriginalStorage.c_str();
-  }
+  WithEnv Env("HOME", nullptr);
 
   // Don't run the test if we have nothing to compare against.
   struct passwd *pw = getpwuid(getuid());
   if (!pw || !pw->pw_dir) return;
-
-  ::unsetenv("HOME");
-  EXPECT_EQ(nullptr, ::getenv("HOME"));
   std::string PwDir = pw->pw_dir;
 
   SmallString<128> HomeDir;
-  auto status = path::home_directory(HomeDir);
-  EXPECT_TRUE(status);
+  EXPECT_TRUE(path::home_directory(HomeDir));
   EXPECT_EQ(PwDir, HomeDir);
+}
+
+TEST(Support, CacheDirectoryWithEnv) {
+  WithEnv Env("XDG_CACHE_HOME", "/xdg/cache");
+
+  SmallString<128> CacheDir;
+  EXPECT_TRUE(path::cache_directory(CacheDir));
+  EXPECT_EQ("/xdg/cache", CacheDir);
+}
+
+TEST(Support, CacheDirectoryNoEnv) {
+  WithEnv Env("XDG_CACHE_HOME", nullptr);
 
-  // Now put the environment back to its original state (meaning that if it was
-  // unset before, we don't reset it).
-  if (OriginalEnv) ::setenv("HOME", OriginalEnv, 1);
+  SmallString<128> Fallback;
+  ASSERT_TRUE(path::home_directory(Fallback));
+  path::append(Fallback, ".cache");
+
+  SmallString<128> CacheDir;
+  EXPECT_TRUE(path::cache_directory(CacheDir));
+  EXPECT_EQ(Fallback, CacheDir);
+}
+#endif
+
+#ifdef _WIN32
+TEST(Support, CacheDirectory) {
+  std::string Expected = getEnvWin(L"LOCALAPPDATA");
+  // Do not try to test it if we don't know what to expect.
+  if (!Expected.empty()) {
+SmallString<128> CacheDir;
+EXPECT_TRUE(path::cache_directory(CacheDir));
+EXPECT_EQ(Expected, CacheDir);
+  }
 }
 #endif
 
Index: llvm/lib/Support/Windows/Path.inc
===
--- llvm/lib/Support/Windows/Path.inc
+++ llvm/lib/Support/Windows/Path.inc
@@ -1372,6 +1372,10 @@
   return getKnownFolderPath(FOLDERID_Profile, result);
 }
 
+bool cache_directory(SmallVectorImpl ) {
+  return getKnownFolderPath(FOLDERID_LocalAppData, result);
+}
+
 static bool getTempDirEnvVar(const wchar_t *Var, SmallVectorImpl ) {
   SmallVector Buf;
   size_t Size = 1024;
Index: llvm/lib/Support/Unix/Path.inc
===
--- llvm/lib/Support/Unix/Path.inc
+++ llvm/lib/Support/Unix/Path.inc
@@ -1138,6 +1138,19 @@
   return true;
 }
 
+bool cache_directory(SmallVectorImpl ) {
+  if (const char *RequestedDir = getenv("XDG_CACHE_HOME")) {
+result.clear();
+result.append(RequestedDir, 

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Sorry about the delay, ran into some problems with the tests and decided to 
simplify them a little by extracting the helper.

Also I think the subdirectory inside $XDG_CACHE_HOME isn't supposed to be 
hidden, so .clangd->clangd which meant a little refactoring.

Thanks for the patch! Landed as 
https://github.com/llvm/llvm-project/commit/ad38f4b371bdca214e3a3cda9a76ec2213215c68


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

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

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 260768.
sammccall added a comment.

Use $cache/clangd/index, not $cache/.clangd/index


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78501/new/

https://reviews.llvm.org/D78501

Files:
  clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp
  llvm/include/llvm/Support/Path.h
  llvm/lib/Support/Unix/Path.inc
  llvm/lib/Support/Windows/Path.inc
  llvm/unittests/Support/Path.cpp

Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -13,6 +13,7 @@
 #include "llvm/ADT/Triple.h"
 #include "llvm/BinaryFormat/Magic.h"
 #include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Compiler.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/ErrorHandling.h"
@@ -305,15 +306,46 @@
   }
 }
 
-TEST(Support, HomeDirectory) {
-  std::string expected;
 #ifdef _WIN32
-  if (wchar_t const *path = ::_wgetenv(L"USERPROFILE")) {
+std::string getEnvWin(const wchar_t *Var) {
+  std::string expected;
+  if (wchar_t const *path = ::_wgetenv(Var)) {
 auto pathLen = ::wcslen(path);
 ArrayRef ref{reinterpret_cast(path),
pathLen * sizeof(wchar_t)};
 convertUTF16ToUTF8String(ref, expected);
   }
+  return expected;
+}
+#else
+// RAII helper to set and restore an environment variable.
+class WithEnv {
+  const char *Var;
+  llvm::Optional OriginalValue;
+
+public:
+  WithEnv(const char *Var, const char *Value) : Var(Var) {
+if (const char *V = ::getenv(Var))
+  OriginalValue.emplace(V);
+if (Value)
+  ::setenv(Var, Value, 1);
+else
+  ::unsetenv(Var);
+set(Value);
+  }
+  ~WithEnv() {
+if (OriginalValue)
+  ::setenv(Var, OriginalValue->c_str(), 1);
+else
+  ::unsetenv(Var);
+  }
+};
+#endif
+
+TEST(Support, HomeDirectory) {
+  std::string expected;
+#ifdef _WIN32
+  expected = getEnvWin(L"USERPROFILE");
 #else
   if (char const *path = ::getenv("HOME"))
 expected = path;
@@ -330,31 +362,48 @@
 
 #ifdef LLVM_ON_UNIX
 TEST(Support, HomeDirectoryWithNoEnv) {
-  std::string OriginalStorage;
-  char const *OriginalEnv = ::getenv("HOME");
-  if (OriginalEnv) {
-// We're going to unset it, so make a copy and save a pointer to the copy
-// so that we can reset it at the end of the test.
-OriginalStorage = OriginalEnv;
-OriginalEnv = OriginalStorage.c_str();
-  }
+  WithEnv Env("HOME", nullptr);
 
   // Don't run the test if we have nothing to compare against.
   struct passwd *pw = getpwuid(getuid());
   if (!pw || !pw->pw_dir) return;
-
-  ::unsetenv("HOME");
-  EXPECT_EQ(nullptr, ::getenv("HOME"));
   std::string PwDir = pw->pw_dir;
 
   SmallString<128> HomeDir;
-  auto status = path::home_directory(HomeDir);
-  EXPECT_TRUE(status);
+  EXPECT_TRUE(path::home_directory(HomeDir));
   EXPECT_EQ(PwDir, HomeDir);
+}
+
+TEST(Support, CacheDirectoryWithEnv) {
+  WithEnv Env("XDG_CACHE_HOME", "/xdg/cache");
+
+  SmallString<128> CacheDir;
+  EXPECT_TRUE(path::cache_directory(CacheDir));
+  EXPECT_EQ("/xdg/cache", CacheDir);
+}
+
+TEST(Support, CacheDirectoryNoEnv) {
+  WithEnv Env("XDG_CACHE_HOME", nullptr);
 
-  // Now put the environment back to its original state (meaning that if it was
-  // unset before, we don't reset it).
-  if (OriginalEnv) ::setenv("HOME", OriginalEnv, 1);
+  SmallString<128> Fallback;
+  ASSERT_TRUE(path::home_directory(Fallback));
+  path::append(Fallback, ".cache");
+
+  SmallString<128> CacheDir;
+  EXPECT_TRUE(path::cache_directory(CacheDir));
+  EXPECT_EQ(Fallback, CacheDir);
+}
+#endif
+
+#ifdef _WIN32
+TEST(Support, CacheDirectory) {
+  std::string Expected = getEnvWin(L"LOCALAPPDATA");
+  // Do not try to test it if we don't know what to expect.
+  if (!Expected.empty()) {
+SmallString<128> CacheDir;
+EXPECT_TRUE(path::cache_directory(CacheDir));
+EXPECT_EQ(Expected, CacheDir);
+  }
 }
 #endif
 
Index: llvm/lib/Support/Windows/Path.inc
===
--- llvm/lib/Support/Windows/Path.inc
+++ llvm/lib/Support/Windows/Path.inc
@@ -1372,6 +1372,10 @@
   return getKnownFolderPath(FOLDERID_Profile, result);
 }
 
+bool cache_directory(SmallVectorImpl ) {
+  return getKnownFolderPath(FOLDERID_LocalAppData, result);
+}
+
 static bool getTempDirEnvVar(const wchar_t *Var, SmallVectorImpl ) {
   SmallVector Buf;
   size_t Size = 1024;
Index: llvm/lib/Support/Unix/Path.inc
===
--- llvm/lib/Support/Unix/Path.inc
+++ llvm/lib/Support/Unix/Path.inc
@@ -1138,6 +1138,19 @@
   return true;
 }
 
+bool cache_directory(SmallVectorImpl ) {
+  if (const char *RequestedDir = getenv("XDG_CACHE_HOME")) {
+result.clear();
+result.append(RequestedDir, RequestedDir + strlen(RequestedDir));
+return true;
+  }
+  if (!home_directory(result)) {
+

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

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 260757.
sammccall added a comment.

Fix(?) test compile for windows, split out windows/unix tests for simplicity.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78501/new/

https://reviews.llvm.org/D78501

Files:
  clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp
  llvm/include/llvm/Support/Path.h
  llvm/lib/Support/Unix/Path.inc
  llvm/lib/Support/Windows/Path.inc
  llvm/unittests/Support/Path.cpp

Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -13,6 +13,7 @@
 #include "llvm/ADT/Triple.h"
 #include "llvm/BinaryFormat/Magic.h"
 #include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Compiler.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/ErrorHandling.h"
@@ -305,15 +306,46 @@
   }
 }
 
-TEST(Support, HomeDirectory) {
-  std::string expected;
 #ifdef _WIN32
-  if (wchar_t const *path = ::_wgetenv(L"USERPROFILE")) {
+std::string getEnvWin(const wchar_t *Var) {
+  std::string expected;
+  if (wchar_t const *path = ::_wgetenv(Var)) {
 auto pathLen = ::wcslen(path);
 ArrayRef ref{reinterpret_cast(path),
pathLen * sizeof(wchar_t)};
 convertUTF16ToUTF8String(ref, expected);
   }
+  return expected;
+}
+#else
+// RAII helper to set and restore an environment variable.
+class WithEnv {
+  const char *Var;
+  llvm::Optional OriginalValue;
+
+public:
+  WithEnv(const char *Var, const char *Value) : Var(Var) {
+if (const char *V = ::getenv(Var))
+  OriginalValue.emplace(V);
+if (Value)
+  ::setenv(Var, Value, 1);
+else
+  ::unsetenv(Var);
+set(Value);
+  }
+  ~WithEnv() {
+if (OriginalValue)
+  ::setenv(Var, OriginalValue->c_str(), 1);
+else
+  ::unsetenv(Var);
+  }
+};
+#endif
+
+TEST(Support, HomeDirectory) {
+  std::string expected;
+#ifdef _WIN32
+  expected = getEnvWin(L"USERPROFILE");
 #else
   if (char const *path = ::getenv("HOME"))
 expected = path;
@@ -330,31 +362,48 @@
 
 #ifdef LLVM_ON_UNIX
 TEST(Support, HomeDirectoryWithNoEnv) {
-  std::string OriginalStorage;
-  char const *OriginalEnv = ::getenv("HOME");
-  if (OriginalEnv) {
-// We're going to unset it, so make a copy and save a pointer to the copy
-// so that we can reset it at the end of the test.
-OriginalStorage = OriginalEnv;
-OriginalEnv = OriginalStorage.c_str();
-  }
+  WithEnv Env("HOME", nullptr);
 
   // Don't run the test if we have nothing to compare against.
   struct passwd *pw = getpwuid(getuid());
   if (!pw || !pw->pw_dir) return;
-
-  ::unsetenv("HOME");
-  EXPECT_EQ(nullptr, ::getenv("HOME"));
   std::string PwDir = pw->pw_dir;
 
   SmallString<128> HomeDir;
-  auto status = path::home_directory(HomeDir);
-  EXPECT_TRUE(status);
+  EXPECT_TRUE(path::home_directory(HomeDir));
   EXPECT_EQ(PwDir, HomeDir);
+}
+
+TEST(Support, CacheDirectoryWithEnv) {
+  WithEnv Env("XDG_CACHE_HOME", "/xdg/cache");
+
+  SmallString<128> CacheDir;
+  EXPECT_TRUE(path::cache_directory(CacheDir));
+  EXPECT_EQ("/xdg/cache", CacheDir);
+}
+
+TEST(Support, CacheDirectoryNoEnv) {
+  WithEnv Env("XDG_CACHE_HOME", nullptr);
 
-  // Now put the environment back to its original state (meaning that if it was
-  // unset before, we don't reset it).
-  if (OriginalEnv) ::setenv("HOME", OriginalEnv, 1);
+  SmallString<128> Fallback;
+  ASSERT_TRUE(path::home_directory(Fallback));
+  path::append(Fallback, ".cache");
+
+  SmallString<128> CacheDir;
+  EXPECT_TRUE(path::cache_directory(CacheDir));
+  EXPECT_EQ(Fallback, CacheDir);
+}
+#endif
+
+#ifdef _WIN32
+TEST(Support, CacheDirectory) {
+  std::string Expected = getEnvWin(L"LOCALAPPDATA");
+  // Do not try to test it if we don't know what to expect.
+  if (!Expected.empty()) {
+SmallString<128> CacheDir;
+EXPECT_TRUE(path::cache_directory(CacheDir));
+EXPECT_EQ(Expected, CacheDir);
+  }
 }
 #endif
 
Index: llvm/lib/Support/Windows/Path.inc
===
--- llvm/lib/Support/Windows/Path.inc
+++ llvm/lib/Support/Windows/Path.inc
@@ -1372,6 +1372,10 @@
   return getKnownFolderPath(FOLDERID_Profile, result);
 }
 
+bool cache_directory(SmallVectorImpl ) {
+  return getKnownFolderPath(FOLDERID_LocalAppData, result);
+}
+
 static bool getTempDirEnvVar(const wchar_t *Var, SmallVectorImpl ) {
   SmallVector Buf;
   size_t Size = 1024;
Index: llvm/lib/Support/Unix/Path.inc
===
--- llvm/lib/Support/Unix/Path.inc
+++ llvm/lib/Support/Unix/Path.inc
@@ -1138,6 +1138,19 @@
   return true;
 }
 
+bool cache_directory(SmallVectorImpl ) {
+  if (const char *RequestedDir = getenv("XDG_CACHE_HOME")) {
+result.clear();
+result.append(RequestedDir, RequestedDir + strlen(RequestedDir));
+return true;
+  }
+  if 

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

[Vojtěch Štěpančík via Phabricator via cfe-commits]

VojtechStep added a comment.
Herald added a project: LLVM.

Ping @sammccall, could you commit the changes? Thanks 


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

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

[Vojtěch Štěpančík via Phabricator via cfe-commits]

VojtechStep updated this revision to Diff 258805.
VojtechStep edited the summary of this revision.
VojtechStep added a comment.

@sammccall I think I fixed all raised issues. Let me know if there is anything 
else I can do, otherwise feel free to commit it.

Also Note to self: Once this is pushed to Arch packages we should add Clangd to 
the Supported list of XDG Base Dir Spec support on Arch Wiki 
.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78501/new/

https://reviews.llvm.org/D78501

Files:
  clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp
  llvm/include/llvm/Support/Path.h
  llvm/lib/Support/Unix/Path.inc
  llvm/lib/Support/Windows/Path.inc
  llvm/unittests/Support/Path.cpp

Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -358,6 +358,122 @@
 }
 #endif
 
+TEST(Support, CacheDirectoryWithEnv) {
+  std::string Expected;
+  // The value of the environment variable is set to a non-standard
+  // location for the test, so we have to store the original value
+  // even if it's not set.
+  // Use an std::string to copy the data
+#ifdef _WIN32
+  Optional OriginalStorage;
+  if (wchar_t const *path = ::_wgetenv(L"XDG_CACHE_HOME")) {
+OriginalStorage = path;
+  }
+
+  ::_wputenv("XDG_CACHE_HOME", L"C:\\xdg\\cache");
+
+  if (wchar_t const *localAppData = ::_wgetenv(L"LOCALAPPDATA")) {
+auto pathLen = ::wcslen(path);
+ArrayRef ref{reinterpret_cast(path),
+   pathLen * sizeof(wchar_t)};
+convertUTF16ToUTF8String(ref, Expected);
+  }
+
+  if (!Expected.empty()) {
+SmallString<128> CacheDir;
+auto status = path::cache_directory(CacheDir);
+EXPECT_TRUE(status);
+EXPECT_EQ(Expected, CacheDir);
+  }
+
+  if (OriginalStorage.hasValue()) {
+::_wputenv("XDG_CACHE_HOME", OriginalStorage->c_str());
+  } else {
+::_wputenv("XDG_CACHE_HOME", nullptr);
+  }
+#else
+  Optional OriginalStorage;
+  if (char const *path = ::getenv("XDG_CACHE_HOME")) {
+OriginalStorage = path;
+  }
+
+  Expected = "/xdg/cache";
+  ::setenv("XDG_CACHE_HOME", Expected.c_str(), 1);
+  EXPECT_EQ(Expected, std::string(::getenv("XDG_CACHE_HOME")));
+
+  SmallString<128> CacheDir;
+  auto status = path::cache_directory(CacheDir);
+  EXPECT_TRUE(status);
+  EXPECT_EQ(Expected, CacheDir);
+
+  if (OriginalStorage.hasValue()) {
+::setenv("XDG_CACHE_HOME", OriginalStorage->c_str(), 1);
+  } else {
+::unsetenv("XDG_CACHE_HOME");
+  }
+#endif
+}
+
+TEST(Support, CacheDirectoryNoEnv) {
+  std::string Expected;
+#ifdef _WIN32
+  Optional OriginalStorage;
+  if (wchar_t const *path = ::_wgetenv(L"XDG_CACHE_HOME")) {
+OriginalStorage = path;
+  }
+
+  ::_wputenv("XDG_CACHE_HOME", nullptr);
+  EXPECT_EQ(nullptr, ::_wgetenv("XDG_CACHE_HOME"));
+
+  if (wchar_t const *localAppData = ::_wgetenv(L"LOCALAPPDATA")) {
+auto pathLen = ::wcslen(path);
+ArrayRef ref {reinterpret_cast(path),
+   pathLen * sizeof(wchar_t)};
+convertUTF16ToUTF8String(ref, Expected);
+  }
+
+  // LocalAppData should always be set, but better safe than sorry
+  if (!Expected.empty()) {
+SmallString<128> CacheDir;
+auto status = path::cache_directory(CacheDir);
+EXPECT_TRUE(status);
+EXPECT_EQ(Expected, CacheDir);
+  }
+
+  if (OriginalStorage.hasValue()) {
+::_wputenv("XDG_CACHE_HOME", OriginalStorage->c_str());
+  } else {
+::_wputenv("XDG_CACHE_HOME", nullptr);
+  }
+#else
+  Optional OriginalStorage;
+  if (char const *path = ::getenv("XDG_CACHE_HOME")) {
+OriginalStorage = path;
+  }
+
+  ::unsetenv("XDG_CACHE_HOME");
+  EXPECT_EQ(nullptr, ::getenv("XDG_CACHE_HOME"));
+
+  SmallString<128> HomeDir;
+
+  auto home_status = path::home_directory(HomeDir);
+  EXPECT_TRUE(home_status);
+  path::append(HomeDir, ".cache");
+  Expected = HomeDir.str().str();
+
+  SmallString<128> CacheDir;
+  auto status = path::cache_directory(CacheDir);
+  EXPECT_TRUE(status);
+  EXPECT_EQ(Expected, CacheDir);
+
+  if (OriginalStorage.hasValue()) {
+::setenv("XDG_CACHE_HOME", OriginalStorage->c_str(), 1);
+  } else {
+::unsetenv("XDG_CACHE_HOME");
+  }
+#endif
+}
+
 TEST(Support, TempDirectory) {
   SmallString<32> TempDir;
   path::system_temp_directory(false, TempDir);
Index: llvm/lib/Support/Windows/Path.inc
===
--- llvm/lib/Support/Windows/Path.inc
+++ llvm/lib/Support/Windows/Path.inc
@@ -1372,6 +1372,10 @@
   return getKnownFolderPath(FOLDERID_Profile, result);
 }
 
+bool cache_directory(SmallVectorImpl ) {
+  return getKnownFolderPath(FOLDERID_LocalAppData, result);
+}
+
 static bool getTempDirEnvVar(const wchar_t *Var, SmallVectorImpl ) {
   SmallVector Buf;
   size_t Size = 1024;
Index: llvm/lib/Support/Unix/Path.inc

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

[Sam McCall via Phabricator via cfe-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 );

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 *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 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

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

[Vojtěch Štěpančík via Phabricator via cfe-commits]

VojtechStep created this revision.
VojtechStep added reviewers: sammccall, chandlerc, Bigcheese.
VojtechStep added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, usaxena95, ormris, kadircet, arphaman, 
dexonsmith, jkorous, MaskRay, ilya-biryukov, hiraditya.
Herald added a project: clang.

This patch adds a function that is similar to 
`llvm::sys::path::home_directory`, but provides access to the system cache 
directory.

For Windows, that is %LOCALAPPDATA%, and applications should put their files 
under %LOCALAPPDATA%\Organization\Product\.

For *nixes, it adheres to the XDG Base Directory Specification, so it first 
looks at the XDG_CACHE_HOME environment variable and falls back to ~/.cache/.

Subsequently, the Clangd Index storage leverages this new API to put index 
files somewhere else than the users home directory.

Previous discussion is here 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78501

Files:
  clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp
  llvm/include/llvm/Support/Path.h
  llvm/lib/Support/Unix/Path.inc
  llvm/lib/Support/Windows/Path.inc
  llvm/unittests/Support/Path.cpp

Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -358,6 +358,136 @@
 }
 #endif
 
+TEST(Support, CacheDirectoryWithEnv) {
+
+  std::string expected;
+
+  // The value of the environment variable is set to a non-standard
+  // location for the test, so we have to store the original value
+  // even if it's not set.
+  // Use an std::string to copy the data
+
+#ifdef _WIN32
+  std::wstring OriginalStorage;
+  if (wchar_t const *path = ::_wgetenv(L"XDG_CACHE_HOME")) {
+OriginalStorage = path;
+  }
+
+  ::_wputenv("XDG_CACHE_HOME", L"C:\\xdg\\cache");
+
+  if (wchar_t const *localAppData = ::_wgetenv(L"LOCALAPPDATA")) {
+auto pathLen = ::wcslen(path);
+ArrayRef ref{reinterpret_cast(path),
+   pathLen * sizeof(wchar_t)};
+convertUTF16ToUTF8String(ref, expected);
+  }
+
+  // LocalAppData should always be set, but better safe than sorry
+  if (!expected.empty()) {
+SmallString<128> CacheDir;
+auto status = path::cache_directory(CacheDir);
+EXPECT_TRUE(status);
+EXPECT_EQ(expected, CacheDir);
+  }
+
+  if (!OriginalStorage.empty()) {
+::_wputenv("XDG_CACHE_HOME", OriginalStorage.c_str());
+  } else {
+::_wputenv("XDG_CACHE_HOME", nullptr);
+  }
+#else
+  std::string OriginalStorage;
+  if (char const *path = ::getenv("XDG_CACHE_HOME")) {
+OriginalStorage = path;
+  }
+
+  expected = "/xdg/cache";
+  ::setenv("XDG_CACHE_HOME", expected.c_str(), 1);
+  EXPECT_EQ(expected, std::string(::getenv("XDG_CACHE_HOME")));
+
+  {
+SmallString<128> CacheDir;
+auto status = path::cache_directory(CacheDir);
+EXPECT_TRUE(status);
+EXPECT_EQ(expected, CacheDir);
+  }
+
+  if (!OriginalStorage.empty()) {
+::setenv("XDG_CACHE_HOME", OriginalStorage.c_str(), 1);
+  } else {
+::unsetenv("XDG_CACHE_HOME");
+  }
+#endif
+}
+
+TEST(Support, CacheDirectoryNoEnv) {
+  std::string expected;
+#ifdef _WIN32
+  std::wstring OriginalStorage;
+  if (wchar_t const *path = ::_wgetenv(L"XDG_CACHE_HOME")) {
+OriginalStorage = path;
+  }
+
+  ::_wputenv("XDG_CACHE_HOME", nullptr);
+  EXPECT_EQ(nullptr, ::_wgetenv("XDG_CACHE_HOME"));
+
+  if (wchar_t const *localAppData = ::_wgetenv(L"LOCALAPPDATA")) {
+auto pathLen = ::wcslen(path);
+ArrayRef ref{reinterpret_cast(path),
+   pathLen * sizeof(wchar_t)};
+convertUTF16ToUTF8String(ref, expected);
+  }
+
+  // LocalAppData should always be set, but better safe than sorry
+  if (!expected.empty()) {
+SmallString<128> CacheDir;
+auto status = path::cache_directory(CacheDir);
+EXPECT_TRUE(status);
+EXPECT_EQ(expected, CacheDir);
+  }
+
+  if (!OriginalStorage.empty()) {
+::_wputenv("XDG_CACHE_HOME", OriginalStorage.c_str());
+  } else {
+::_wputenv("XDG_CACHE_HOME", nullptr);
+  }
+#else
+  std::string OriginalStorage;
+  if (char const *path = ::getenv("XDG_CACHE_HOME")) {
+OriginalStorage = path;
+  }
+
+  ::unsetenv("XDG_CACHE_HOME");
+  EXPECT_EQ(nullptr, ::getenv("XDG_CACHE_HOME"));
+
+  SmallString<128> HomeDir;
+  if (path::home_directory(HomeDir)) {
+path::append(HomeDir, ".cache");
+expected = std::string(HomeDir.str());
+  }
+
+  if (!expected.empty()) {
+SmallString<128> CacheDir;
+auto status = path::cache_directory(CacheDir);
+EXPECT_TRUE(status);
+EXPECT_EQ(expected, CacheDir);
+  }
+
+  {
+SmallString<128> CacheDir;
+auto status = path::cache_directory(CacheDir);
+EXPECT_TRUE(status);
+EXPECT_EQ(expected, CacheDir);
+  }
+
+  if (!OriginalStorage.empty()) {
+::setenv("XDG_CACHE_HOME", OriginalStorage.c_str(), 1);
+  } else {
+::unsetenv("XDG_CACHE_HOME");
+  }
+#endif
+}
+