[PATCH] Introduce llvm::sys::path::home_directory.

[Aaron Ballman]

Good point on the MAX_PATH, thank you for the clarification.

~Aaron

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