<div dir="rtl"><div dir="ltr">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).</div><div dir="ltr">Most of LLVM that uses llvm::sys::path do work this way now except some hardcoded seperators here and there.<br>
</div><div dir="ltr">After this patch there would be two less of them...</div><div dir="ltr"><br></div><div dir="ltr"><br></div><div dir="ltr"><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote"><div dir="ltr">
2014-05-09 14:24 GMT+03:00 Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span>:</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><br>
On 09/05/2014 11:53, Yaron Keren wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
<br>
The alternative is to have all path seperators as '/', but this disagrees with path.cpp functionality and all its many clients.<br>
<br>
Alp, is that whan you meant?<br>
</blockquote>
<br></div>
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.<br>
<br>
(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?)<br>
<br>
Alp.<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
2014-05-09 13:21 GMT+03:00 Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a> <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>>:<div><div class="h5"><br>
<br>
<br>
On 09/05/2014 10:32, Yaron Keren wrote:<br>
<br>
Hi echristo, rnk, chapuni,<br>
<br>
Earlier duscussion:<br>
<a href="https://www.mail-archive.com/cfe-commits@cs.uiuc.edu/msg92891.html" target="_blank">https://www.mail-archive.com/<u></u>cfe-commits@cs.uiuc.edu/<u></u>msg92891.html</a><br>
<br>
Most of LLVM code uses the llvm::sys::path::append function to<br>
concatenate paths and will get the correct native slash. In<br>
some code locations it seems that direct string concatenation<br>
was used, possibly because the code writer knows there are no<br>
extra path sepeartors to deal with, so string concat was more<br>
efficient than append.<br>
<br>
In the locations below, the path seperator is hardcoded as<br>
'/'. Fixed by exposing the native seperator from<br>
llvm::sys:;path and using it.<br>
<br>
As an alternative, the append function could be used but it's<br>
more code and less efficient.<br>
<br>
<a href="http://reviews.llvm.org/D3687" target="_blank">http://reviews.llvm.org/D3687</a><br>
<br>
Files:<br>
include/llvm/Support/Path.h<br>
lib/MC/MCDwarf.cpp<br>
lib/Support/Path.cpp<br>
lib/Support/SourceMgr.cpp<br>
<br>
Index: include/llvm/Support/Path.h<br>
==============================<u></u>==============================<u></u>=======<br>
--- include/llvm/Support/Path.h<br>
+++ include/llvm/Support/Path.h<br>
@@ -295,6 +295,11 @@<br>
/// @result true if \a value is a path separator character<br>
on the host OS<br>
bool is_separator(char value);<br>
+/// @brief Return the preferred separator for this platform.<br>
+///<br>
+/// @result const char* to the preferred separator,<br>
null-terminated.<br>
+const char *get_separator();<br>
+<br>
/// @brief Get the typical temporary directory for the<br>
system, e.g.,<br>
/// "/var/tmp" or "C:/TEMP"<br>
///<br>
Index: lib/Support/Path.cpp<br>
==============================<u></u>==============================<u></u>=======<br>
--- lib/Support/Path.cpp<br>
+++ lib/Support/Path.cpp<br>
@@ -569,6 +569,12 @@<br>
}<br>
}<br>
+static const char preferred_separator_string[] = {<br>
preferred_separator, '\0' };<br>
+<br>
+const char *get_separator() {<br>
+ return preferred_separator_string;<br>
+}<br>
+<br>
void system_temp_directory(bool erasedOnReboot,<br>
SmallVectorImpl<char> &result) {<br>
result.clear();<br>
Index: lib/MC/MCDwarf.cpp<br>
==============================<u></u>==============================<u></u>=======<br>
--- lib/MC/MCDwarf.cpp<br>
+++ lib/MC/MCDwarf.cpp<br>
@@ -689,7 +689,7 @@<br>
const SmallVectorImpl<std::string> &MCDwarfDirs =<br>
context.getMCDwarfDirs();<br>
if (MCDwarfDirs.size() > 0) {<br>
MCOS->EmitBytes(MCDwarfDirs[0]<u></u>);<br>
- MCOS->EmitBytes("/");<br>
+ MCOS->EmitBytes(sys::path::<u></u>get_separator());<br>
}<br>
const SmallVectorImpl<MCDwarfFile> &MCDwarfFiles =<br>
MCOS->getContext().<u></u>getMCDwarfFiles();<br>
<br>
<br>
I know there isn't much clarity yet on the issue but it's worth<br>
mentioning (in the commit message or a comment) the current state<br>
of affairs regarding paths in cross-compiled debug info.<br>
<br>
Alp.<br>
<br>
<br>
<br>
Index: lib/Support/SourceMgr.cpp<br>
==============================<u></u>==============================<u></u>=======<br>
--- lib/Support/SourceMgr.cpp<br>
+++ lib/Support/SourceMgr.cpp<br>
@@ -18,6 +18,7 @@<br>
#include "llvm/ADT/Twine.h"<br>
#include "llvm/Support/Locale.h"<br>
#include "llvm/Support/MemoryBuffer.h"<br>
+#include "llvm/Support/Path.h"<br>
#include "llvm/Support/raw_ostream.h"<br>
#include "llvm/Support/system_error.h"<br>
using namespace llvm;<br>
@@ -60,7 +61,7 @@<br>
// If the file didn't exist directly, see if it's in an<br>
include path.<br>
for (unsigned i = 0, e = IncludeDirectories.size(); i != e<br>
&& !NewBuf; ++i) {<br>
- IncludedFile = IncludeDirectories[i] + "/" + Filename;<br>
+ IncludedFile = IncludeDirectories[i] +<br>
sys::path::get_separator() + Filename;<br>
MemoryBuffer::getFile(<u></u>IncludedFile.c_str(), NewBuf);<br>
}<br>
<br>
<br>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br></div></div>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a> <mailto:<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.<u></u>edu</a>><div class=""><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
<br>
<br>
-- <a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
<br>
</div></blockquote><div class="HOEnZb"><div class="h5">
<br>
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</div></div></blockquote></div><br></div>