[PATCH] D13801: [Support] Extend sys::path with user_cache_directory function.

[Paweł Bylica via llvm-commits]

chfast added inline comments.

================
Comment at: include/llvm/Support/Path.h:332
@@ +331,3 @@
+/// applications/services used by the user. At least an application/service name
+/// should be appended to the path before storing any files.
+///
----------------
rafael wrote:
> Maybe take a twine argument and do the appending in here? It is easy to forget to read documentation :-).
That can be done. I would go for 2 Twine optional args to support <vendor>/<application> convention.

But that duplicates features of path::append and you still need to append a file name. The common pattern is:
```
if (sys::path::user_cache_directory(Dir)) {
  sys::path::append(Dir, "LLVM", "Kaleidoscope", CacheFileName);
}
```
and the above would be replaced with:
```
if (sys::path::user_cache_directory(Dir, "LLVM", "Kaleidoscope")) {
  sys::path::append(Dir, CacheFileName);
}
```

Which one is better?

================
Comment at: lib/Support/Unix/Path.inc:636
@@ -585,3 +635,3 @@
 
 void system_temp_directory(bool ErasedOnReboot, SmallVectorImpl<char> &Result) {
   Result.clear();
----------------
rafael wrote:
> Should we drop the EraseOnReboot argument from this function? We would now have
> 
>  user_cache_directory
>  system_temp_directory(true
>  system_temp_directory(false
> 
> When should each one be used?
user_cache_directory is similar to system_temp_directory(false) (level of similarity is OS-specific). We can drop system_temp_directory(false) then, but because it is the API change it's probably bad idea to do it at once.

In the end I would like to have:
user_cache_directory
system_temp_directory
and maybe user_temp_directory

I think that needs to be discussed. And we can agree on the final solution I'm happy to implement it.


http://reviews.llvm.org/D13801