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

[David Majnemer]

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131117/e1a24ff1/attachment.html>