<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sun, Nov 17, 2013 at 12:21 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">On Sun, Nov 17, 2013 at 5:55 AM, Peter Collingbourne <<a href="mailto:peter@pcc.me.uk">peter@pcc.me.uk</a>> wrote:<br>
> This will be used by the line editor library to derive a default path to<br>
> the history file.<br>
><br>
> I've verified that the Windows bits build (with a mingw64 cross compiler)<br>
> but I haven't actually tested them.<br>
><br>
> <a href="http://llvm-reviews.chandlerc.com/D2199" target="_blank">http://llvm-reviews.chandlerc.com/D2199</a><br>
><br>
> Files:<br>
> include/llvm/Support/Path.h<br>
> lib/Support/Unix/Path.inc<br>
> lib/Support/Windows/Path.inc<br>
> unittests/Support/Path.cpp<br>
><br>
> Index: include/llvm/Support/Path.h<br>
> ===================================================================<br>
> --- include/llvm/Support/Path.h<br>
> +++ include/llvm/Support/Path.h<br>
> @@ -306,6 +306,12 @@<br>
> /// @param result Holds the resulting path name.<br>
> void system_temp_directory(bool erasedOnReboot, SmallVectorImpl<char> &result);<br>
><br>
> +/// @brief Get the user's home directory.<br>
> +///<br>
> +/// @param result Holds the resulting path name.<br>
> +/// @result True if a home directory is set, false otherwise.<br>
> +bool home_directory(SmallVectorImpl<char> &result);<br>
> +<br>
> /// @brief Has root name?<br>
> ///<br>
> /// root_name != ""<br>
> Index: lib/Support/Unix/Path.inc<br>
> ===================================================================<br>
> --- lib/Support/Unix/Path.inc<br>
> +++ lib/Support/Unix/Path.inc<br>
> @@ -793,5 +793,19 @@<br>
> }<br>
><br>
> } // end namespace fs<br>
> +<br>
> +namespace path {<br>
> +<br>
> +bool home_directory(SmallVectorImpl<char> &result) {<br>
> + if (char *RequestedDir = getenv("HOME")) {<br>
> + result.append(RequestedDir, RequestedDir + strlen(RequestedDir));<br>
<br>
</div></div>This behavior differs from the Windows implementation in that it's<br>
appending to the results instead of replacing them.<br>
<div><div class="h5"><br>
> + return true;<br>
> + }<br>
> +<br>
> + return false;<br>
> +}<br>
> +<br>
> +} // end namespace path<br>
> +<br>
> } // end namespace sys<br>
> } // end namespace llvm<br>
> Index: lib/Support/Windows/Path.inc<br>
> ===================================================================<br>
> --- lib/Support/Windows/Path.inc<br>
> +++ lib/Support/Windows/Path.inc<br>
> @@ -20,6 +20,7 @@<br>
> #include "Windows.h"<br>
> #include <fcntl.h><br>
> #include <io.h><br>
> +#include <shlobj.h><br>
> #include <sys/stat.h><br>
> #include <sys/types.h><br>
><br>
> @@ -1061,6 +1062,22 @@<br>
> }<br>
> } // end namespace fs<br>
><br>
> +namespace path {<br>
> +<br>
> +bool home_directory(SmallVectorImpl<char> &result) {<br>
> + wchar_t Path[MAX_PATH];<br>
<br>
</div></div>The documentation is a bit cagey on this -- it says "a null terminated<br>
string of length MAX_PATH", which I take to mean you need to allocate<br>
MAX_PATH + 1 characters for the edge case where you're returned a<br>
string of length MAX_PATH.<br></blockquote><div><br></div><div>MAX_PATH is sized to include the terminating null character: <a href="http://msdn.microsoft.com/en-us/library/aa365247(VS.85).aspx#maxpath">http://msdn.microsoft.com/en-us/library/aa365247(VS.85).aspx#maxpath</a></div>
<div><br></div><div>It seems that the documentation is just awkwardly phrased, they should have said "with buffer size MAX_PATH". They also have an example usage on the SHGetFolderPath page that, while not normative, seems to reinforce that we want MAX_PATH, not MAX_PATH+1.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">
> + if (::SHGetFolderPathW(0, CSIDL_APPDATA | CSIDL_FLAG_CREATE, 0,<br>
> + SHGFP_TYPE_CURRENT, Path) != S_OK)<br>
<br>
</div>You shouldn't be passing CSIDL_FLAG_CREATE here; if the user has no<br>
home directory, it should fail as in the unix case. Also, as a total<br>
nitpick, I'd prefer to see use of the FAILED macro instead of checking<br>
the result directly. Eg)<br>
<br>
if (FAILED(::SHGetFolderPath(...))<br>
<div class=""><div class="h5"><br>
> + return false;<br>
> +<br>
> + if (UTF16ToUTF8(Path, ::wcslen(Path), result))<br>
> + return false;<br>
> +<br>
> + return true;<br>
> +}<br>
> +<br>
> +} // end namespace path<br>
> +<br>
> namespace windows {<br>
> llvm::error_code UTF8ToUTF16(llvm::StringRef utf8,<br>
> llvm::SmallVectorImpl<wchar_t> &utf16) {<br>
> Index: unittests/Support/Path.cpp<br>
> ===================================================================<br>
> --- unittests/Support/Path.cpp<br>
> +++ unittests/Support/Path.cpp<br>
> @@ -210,6 +210,19 @@<br>
> }<br>
> #endif // LLVM_ON_WIN32<br>
><br>
> +TEST(Support, HomeDirectory) {<br>
> +#ifdef LLVM_ON_UNIX<br>
> + // This test only makes sense on Unix if $HOME is set.<br>
> + if (::getenv("HOME")) {<br>
> +#endif<br>
> + SmallString<128> HomeDir;<br>
> + EXPECT_TRUE(path::home_directory(HomeDir));<br>
> + EXPECT_FALSE(HomeDir.empty());<br>
> +#ifdef LLVM_ON_UNIX<br>
> + }<br>
> +#endif<br>
> +}<br>
> +<br>
> class FileSystemTest : public testing::Test {<br>
> protected:<br>
> /// Unique temporary directory in which all created filesystem entities must<br>
><br>
</div></div><div class="im">> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
<br>
</div><span class=""><font color="#888888">~Aaron<br>
</font></span><div class=""><div class="h5">_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>