[PATCH] Fix hardcoded path seperator in two code locations
Yaron Keren
yaron.keren at gmail.com
Fri May 9 04:28:56 PDT 2014
I'm for the host path seperator since the files exist on the host file
system and not on the target file system (if exists at all).
Most of LLVM that uses llvm::sys::path do work this way now except some
hardcoded seperators here and there.
After this patch there would be two less of them...
2014-05-09 14:24 GMT+03:00 Alp Toker <alp at nuanti.com>:
>
> On 09/05/2014 11:53, Yaron Keren wrote:
>
>> 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?
>>
>
> Yep, exactly this. I was just curious what tipped you in favour of
> preferring the host path separator and your mail didn't mention that part
> of the change explicitly.
>
> (I'm guessing the right fix will be to some day provide a function that
> maps relative paths based on knowledge of the target OS. Or maybe there is
> no right fix?)
>
> Alp.
>
>
>
>>
>> 2014-05-09 13:21 GMT+03:00 Alp Toker <alp at nuanti.com <mailto:
>> 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 <mailto:llvm-commits at cs.uiuc.edu>
>>
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>> -- http://www.nuanti.com
>> the browser experts
>>
>>
>>
> --
> http://www.nuanti.com
> the browser experts
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140509/0ea30e19/attachment.html>
More information about the llvm-commits
mailing list