[PATCH] Fix hardcoded path seperator in two code locations

[Yaron Keren]

Hi Takumi,

I wish DOS would have used '/' but now it's far too late. Even today it
sometimes causes troubles building on Windows, problems with configure,
make,...

The current state in incosistent, where most of LLVM uses sys::path and
thus '\\' (on WIndows) but some places use hardcodes '/'.

This patch does not make the '\\' or '/' decision but rather centralizes
the decision to llvm::sys::path, so if someday we would like to switch all
to '/' as you like, only path.cpp would be modified.

Yaron



2014-05-09 14:39 GMT+03:00 NAKAMURA Takumi <geek4civic at gmail.com>:

> I prefer '/' could be used everywhere, unless it were illegal. I think
> '\\' makes the world worse.
>
> 2014-05-09 19:53 GMT+09:00 Yaron Keren <yaron.keren at gmail.com>:
> > In the discussion linked, Reid mentioned that when cross-compiling aren't
> > the paths are valid on the host only, so shouldn't the host path
> seperator
> > be used? the target may not even have a file system.
> >
> > The alternative is to have all path seperators as '/', but this disagrees
> > with path.cpp functionality and all its many clients.
> >
> > Alp, is that whan you meant?
> >
> >
> > 2014-05-09 13:21 GMT+03:00 Alp Toker <alp at nuanti.com>:
> >>
> >>
> >> On 09/05/2014 10:32, Yaron Keren wrote:
> >>>
> >>> Hi echristo, rnk, chapuni,
> >>>
> >>> Earlier duscussion:
> >>>   https://www.mail-archive.com/cfe-commits@cs.uiuc.edu/msg92891.html
> >>>
> >>> Most of LLVM code uses the llvm::sys::path::append function to
> >>> concatenate paths and will get the correct native slash. In some code
> >>> locations it seems that direct string concatenation was used, possibly
> >>> because the code writer knows there are no extra path sepeartors to
> deal
> >>> with, so string concat was more efficient than append.
> >>>
> >>> In the locations below, the path seperator is hardcoded as '/'. Fixed
> by
> >>> exposing the native seperator from llvm::sys:;path and using it.
> >>>
> >>> As an alternative, the append function could be used but it's more code
> >>> and less efficient.
> >>>
> >>> http://reviews.llvm.org/D3687
> >>>
> >>> Files:
> >>>    include/llvm/Support/Path.h
> >>>    lib/MC/MCDwarf.cpp
> >>>    lib/Support/Path.cpp
> >>>    lib/Support/SourceMgr.cpp
> >>>
> >>> Index: include/llvm/Support/Path.h
> >>> ===================================================================
> >>> --- include/llvm/Support/Path.h
> >>> +++ include/llvm/Support/Path.h
> >>> @@ -295,6 +295,11 @@
> >>>   /// @result true if \a value is a path separator character on the
> host
> >>> OS
> >>>   bool is_separator(char value);
> >>>   +/// @brief Return the preferred separator for this platform.
> >>> +///
> >>> +/// @result const char* to the preferred separator, null-terminated.
> >>> +const char *get_separator();
> >>> +
> >>>   /// @brief Get the typical temporary directory for the system, e.g.,
> >>>   /// "/var/tmp" or "C:/TEMP"
> >>>   ///
> >>> Index: lib/Support/Path.cpp
> >>> ===================================================================
> >>> --- lib/Support/Path.cpp
> >>> +++ lib/Support/Path.cpp
> >>> @@ -569,6 +569,12 @@
> >>>     }
> >>>   }
> >>>   +static const char preferred_separator_string[] = {
> >>> preferred_separator, '\0' };
> >>> +
> >>> +const char *get_separator() {
> >>> +  return preferred_separator_string;
> >>> +}
> >>> +
> >>>   void system_temp_directory(bool erasedOnReboot, SmallVectorImpl<char>
> >>> &result) {
> >>>     result.clear();
> >>>   Index: lib/MC/MCDwarf.cpp
> >>> ===================================================================
> >>> --- lib/MC/MCDwarf.cpp
> >>> +++ lib/MC/MCDwarf.cpp
> >>> @@ -689,7 +689,7 @@
> >>>     const SmallVectorImpl<std::string> &MCDwarfDirs =
> >>> context.getMCDwarfDirs();
> >>>     if (MCDwarfDirs.size() > 0) {
> >>>       MCOS->EmitBytes(MCDwarfDirs[0]);
> >>> -    MCOS->EmitBytes("/");
> >>> +    MCOS->EmitBytes(sys::path::get_separator());
> >>>     }
> >>>     const SmallVectorImpl<MCDwarfFile> &MCDwarfFiles =
> >>>       MCOS->getContext().getMCDwarfFiles();
> >>
> >>
> >> I know there isn't much clarity yet on the issue but it's worth
> mentioning
> >> (in the commit message or a comment) the current state of affairs
> regarding
> >> paths in cross-compiled debug info.
> >>
> >> Alp.
> >>
> >>
> >>
> >>> Index: lib/Support/SourceMgr.cpp
> >>> ===================================================================
> >>> --- lib/Support/SourceMgr.cpp
> >>> +++ lib/Support/SourceMgr.cpp
> >>> @@ -18,6 +18,7 @@
> >>>   #include "llvm/ADT/Twine.h"
> >>>   #include "llvm/Support/Locale.h"
> >>>   #include "llvm/Support/MemoryBuffer.h"
> >>> +#include "llvm/Support/Path.h"
> >>>   #include "llvm/Support/raw_ostream.h"
> >>>   #include "llvm/Support/system_error.h"
> >>>   using namespace llvm;
> >>> @@ -60,7 +61,7 @@
> >>>       // If the file didn't exist directly, see if it's in an include
> >>> path.
> >>>     for (unsigned i = 0, e = IncludeDirectories.size(); i != e &&
> >>> !NewBuf; ++i) {
> >>> -    IncludedFile = IncludeDirectories[i] + "/" + Filename;
> >>> +    IncludedFile = IncludeDirectories[i] + sys::path::get_separator()
> +
> >>> Filename;
> >>>       MemoryBuffer::getFile(IncludedFile.c_str(), NewBuf);
> >>>     }
> >>>
> >>>
> >>> _______________________________________________
> >>> llvm-commits mailing list
> >>> llvm-commits at cs.uiuc.edu
> >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>
> >>
> >> --
> >> http://www.nuanti.com
> >> the browser experts
> >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140509/feabc2c0/attachment.html>