[PATCH] Turn local DWARFContext helpers getFileNameForUnit() and getFileLineInfoForCompileUnit() into full-blowm DWARFDebugLine::LineTable methods.

Frederic Riss friss at apple.com
Mon Sep 15 11:38:45 PDT 2014


================
Comment at: lib/DebugInfo/DWARFDebugLine.cpp:663
@@ -658,3 +662,3 @@
   }
   SmallString<16> FilePath;
   uint64_t IncludeDirIndex = Entry.DirIdx;
----------------
samsonov wrote:
> Looks like you can have a single SmallString<> to construct an absolute filepath.
You mean removing the AbsolutePath variable in the is_relative code bellow right?

================
Comment at: lib/DebugInfo/DWARFDebugLine.cpp:693
@@ +692,3 @@
+                                                         DILineInfo &Result) const {
+  if (!CU)
+    return false;
----------------
samsonov wrote:
> It's not clear now why CU must be nonzero, even though getFileNameByIndex allows U to be nullptr.
This is just how the original code is written, this patch is supposed to have no functional change. I agree with you that this looks strange, and maybe this check should be extracted and moved to the current callers.

================
Comment at: lib/DebugInfo/DWARFDebugLine.h:184
@@ -181,3 +183,3 @@
     // Returns true on success.
     bool getFileNameByIndex(uint64_t FileIndex,
                             DILineInfoSpecifier::FileLineInfoKind Kind,
----------------
samsonov wrote:
> It's somewhat confusing to have output parameter not the last parameter in the function. See my suggestion about passing compilation dir instead of DWARFUnit* - in this case you can just pass empty string if unit is unavailable. 
Agreed. I mechanically add new parameters with a default value to the end to avoid having to modify the existing callers, but here it looks strange.
I'd rather pass a raw const char * to avoid creating unnecessary string objects though.

http://reviews.llvm.org/D5354






More information about the llvm-commits mailing list