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

[Paweł Bylica via llvm-commits]

chfast added a comment.

Thanks for the review.


================
Comment at: include/llvm/Support/Path.h:328
@@ -327,1 +327,3 @@
 
+/// Get the user's cache directory.
+///
----------------
aaron.ballman wrote:
> Should we be using @brief here like the rest of the file?
@brief has become redundant some time ago as Doxygen's option JAVADOC_AUTOBRIEF has been turned on. The brief is now first line until dot.

================
Comment at: lib/Support/Unix/Path.inc:592
@@ +591,3 @@
+
+  /// First try using XDS_CACHE_HOME env variable,
+  /// as specified in XDG Base Directory Specification at
----------------
aaron.ballman wrote:
> Please use /// only for doxygen comments, // otherwise.
I actually wanted that to be exported as a Doxygen note. But that might not be the best idea as it is Unix specific.

================
Comment at: lib/Support/Windows/Path.inc:773
@@ +772,3 @@
+bool user_cache_directory(SmallVectorImpl<char> &result) {
+  return getKnownFolderPath(FOLDERID_LocalAppData, result);
+}
----------------
aaron.ballman wrote:
> How is this intended to be used? Is the caller expected to place a directory under the user_cache_directory() and use that, or are they expecting this to be a folder where they can dump cached data? I ask because putting files directly into the local AppData directory is not a particularly good thing to do on Windows.
That's right. An application should append its name to the returned path in general case. But that issues is already present on Darwin with `system_temp_directory(false)` that returns a path of "~/Library/Caches".

That should be definitely clarified in the docs.

I'm thinking about adding optional param to the function with a path that will be appended to the returned path. Then usage could be `user_cache_directory(result, "vendor/appname"). What do you think?


http://reviews.llvm.org/D13801