[PATCH] [prototype] Adding line table debug information to LLVM on Windows

[Reid Kleckner]

  Arg, I drafted and lost a whole bunch of comments on this patch.  Sending them out, even though they are now out of date...


================
Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.h:90
@@ +89,3 @@
+  // readily available.
+  typedef DenseMap<std::pair<const char*, const char*>, char *>
+      DirAndFilenameToFilepathMapTy;
----------------
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.

================
Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.h:88
@@ +87,3 @@
+
+  // TODO: we can probably get rid of this map if MDNode holds the fullpath
+  // readily available.
----------------
This TODO seems obsolete now.

================
Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:42-44
@@ +41,5 @@
+  size_t Cursor = 0;
+  // First, replace all slashes with backslashes.
+  while ((Cursor = Filepath.find('/', Cursor)) != std::string::npos)
+    Filepath[Cursor++] = '\\';
+
----------------
I suppose if the user is cross-compiling from Linux targeting Windows, they always want backslashes, so llvm::sys::path::native() is wrong.

You can use its shorter implementation though:
  std::replace(path.begin(), path.end(), '/', '\\')

================
Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:30
@@ +29,3 @@
+  char *&Result =
+      DirAndFilenameToFilepathMap[std::make_pair(Dir.data(), Filename.data())];
+  if (Result != 0)
----------------
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*

================
Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:51-52
@@ +50,4 @@
+
+  // Replace all "\XXX\..\" with "\".  Don't try too hard though as the original
+  // path should be well-formatted, e.g. start with a drive letter, etc.
+  Cursor = 0;
----------------
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.


http://llvm-reviews.chandlerc.com/D2232