This is an archive of the discontinued LLVM Phabricator instance.

Add a facility to get system cache directory and use it in clangd
ClosedPublic

Authored by VojtechStep on Apr 20 2020, 9:02 AM.

Details

Summary

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.

Fixes https://github.com/clangd/clangd/issues/341

Diff Detail

Event Timeline

VojtechStep created this revision.Apr 20 2020, 9:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2020, 9:02 AM
sammccall accepted this revision.Apr 20 2020, 9:33 AM

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.

llvm/include/llvm/Support/Path.h
372

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)

375

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.

llvm/lib/Support/Unix/Path.inc
1142

nit: suggest const char* to signal we're not doing anything bad with this terrifying API :-)

1143

nit: llvm style is to inline this if (char * RequestedDir = ...)

llvm/unittests/Support/Path.cpp
363

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)

426

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 :-))

464

you can just ASSERT_TRUE, this should always succeed in our test setups I think.

469

Why are we testing this twice, once conditionally?

This revision is now accepted and ready to land.Apr 20 2020, 9:33 AM
VojtechStep edited the summary of this revision. (Show Details)

@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.

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

Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2020, 12:28 PM
sammccall updated this revision to Diff 260757.Apr 28 2020, 1:51 PM

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

sammccall updated this revision to Diff 260768.Apr 28 2020, 2:15 PM

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

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

This revision was automatically updated to reflect the committed changes.

Great, thanks for the fixes!