[PATCH] Fix hardcoded path seperator in two code locations

[NAKAMURA Takumi]

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
>>
>