[PATCH] Fix hardcoded path seperator in two code locations

[Yaron Keren]

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/053f7aee/attachment.html>