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

[Aaron Ballman via llvm-commits]

aaron.ballman added a comment.

In http://reviews.llvm.org/D13801#271166, @chfast wrote:

> Aaron, could you comment my comments?


Sorry for the delayed response; I'm in standards meetings this week, and vacation next, so I'm a bit hard to pin down.


================
Comment at: include/llvm/Support/Path.h:328
@@ -327,1 +327,3 @@
 
+/// Get the user's cache directory.
+///
----------------
chfast wrote:
> 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.
I was speaking more for conformity within the same file. We usually try to match the style of the file, even if it's not strictly the correct thing to do style-wise.

I don't feel strongly. I just prefer consistency. Easier on the eyes. ;-)

================
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
----------------
chfast wrote:
> 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.
Good to know. I would still recommend only using //.

================
Comment at: lib/Support/Windows/Path.inc:773
@@ +772,3 @@
+bool user_cache_directory(SmallVectorImpl<char> &result) {
+  return getKnownFolderPath(FOLDERID_LocalAppData, result);
+}
----------------
chfast wrote:
> 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?
I am not certain that's a good idea because then it embeds path separators in the string literal. If there's a way to pass that information that's abstracted (I'm not certain if something in fs would suffice or not), then I think it's a good idea.


http://reviews.llvm.org/D13801