<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">2013/12/16 Reid Kleckner <span dir="ltr"><<a href="mailto:rnk@google.com" target="_blank" class="cremed">rnk@google.com</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
  Arg, I drafted and lost a whole bunch of comments on this patch.  Sending them out, even though they are now out of date...<br></blockquote><div><br></div><div>:(</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


================<br>
Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.h:90<br>
@@ +89,3 @@<br>
+  // readily available.<br>
+  typedef DenseMap<std::pair<const char*, const char*>, char *><br>
+      DirAndFilenameToFilepathMapTy;<br>
----------------<br>
Is this specialized to use string comparison or will it use pointer comparison?  If Dir and Filename are uniqued in the IR pointer comparison could be OK.<br></blockquote><div><br></div><div>Good point - will check.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
================<br>
Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.h:88<br>
@@ +87,3 @@<br>
+<br>
+  // TODO: we can probably get rid of this map if MDNode holds the fullpath<br>
+  // readily available.<br>
----------------<br>
This TODO seems obsolete now.<br></blockquote><div><br></div><div>Yep, it's gone in the newer version of the patch.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


================<br>
Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:42-44<br>
@@ +41,5 @@<br>
+  size_t Cursor = 0;<br>
+  // First, replace all slashes with backslashes.<br>
+  while ((Cursor = Filepath.find('/', Cursor)) != std::string::npos)<br>
+    Filepath[Cursor++] = '\\';<br>
+<br>
----------------<br>
I suppose if the user is cross-compiling from Linux targeting Windows, they always want backslashes, so llvm::sys::path::native() is wrong.<br>
<br>
You can use its shorter implementation though:<br>
  std::replace(path.begin(), path.end(), '/', '\\')<br></blockquote><div><br></div><div>Good catch - will do.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


================<br>
Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:30<br>
@@ +29,3 @@<br>
+  char *&Result =<br>
+      DirAndFilenameToFilepathMap[std::make_pair(Dir.data(), Filename.data())];<br>
+  if (Result != 0)<br>
----------------<br>
Is this map here for performance or reducing memory usage?  I think the simpler way to reduce memory usage is to do the path canonicalization unconditionally, and then intern the string with a StringSet.  Saves a template instantiation.  *shrug*<br>

</blockquote><div><br></div><div>I think this improves performance without sacrificing the memory usage too much.</div><div>We call getFullFilename() for each debug location (think asm instruction), so I think performance is really important here.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
================<br>
Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:51-52<br>
@@ +50,4 @@<br>
+<br>
+  // Replace all "\XXX\..\" with "\".  Don't try too hard though as the original<br>
+  // path should be well-formatted, e.g. start with a drive letter, etc.<br>
+  Cursor = 0;<br>
----------------<br>
I guess we have to do it textually like this because we can't look at the filesystem.  We could be doing distributed cross-compilation, in which case the paths won't actually make sense.<br></blockquote><div><br>

</div><div>Exactly.</div><div>Even running LL lit tests on the same machine after a while makes the filepaths dangle.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


<a href="http://llvm-reviews.chandlerc.com/D2232" target="_blank" class="cremed">http://llvm-reviews.chandlerc.com/D2232</a><br>
</blockquote></div><br></div></div>