tberghammer added a comment.
The high level architecture looks reasonable but there are a few think I would
like you to clean up a bit:
- Several of your function uses a raw "new ...()" and then returns a pointer.
Please put the data into a smart pointer (usually std::unique_ptr) and return
that one instead to eliminate the risk of a memory leak.
- I added a bunch of inline comments. Most of them are minor style issues but
there are some more significant ones (e.g. unused data structures)
How much advantage do you expect from this change from the users point of view?
Do you think we should try to make clang produce .debug_macros section also?
In case of split dwarf gcc produces several .debug_macro.dwo section for each
compile unit what won't work with your current implementation. I haven't
checked the spec if gcc-s behavior is correct or not but it might be a good
idea to support it (not necessary with this change). If you decide to not
support split dwarf then please mark the test as XFAIL(dwo) also
================
Comment at: include/lldb/Symbol/DebugMacros.h:56
@@ +55,3 @@
+
+ ~DebugMacroEntry() { }
+
----------------
(nit): "= default" (for all other similar function also)
================
Comment at: include/lldb/Symbol/DebugMacros.h:131
@@ +130,3 @@
+ else
+ return NULL;
+ }
----------------
(nit): Please use nullptr (for all other similar case also)
================
Comment at: include/lldb/Symbol/DebugMacros.h:137
@@ +136,3 @@
+
+ std::vector<DebugMacroEntryUP> m_macro_entries;
+};
----------------
What is the benefit of storing unique pointers in the vector compared to the
objects itself?
================
Comment at: source/Expression/ExpressionSourceCode.cpp:106
@@ +105,3 @@
+void
+AddMacros(const DebugMacros *dm, AddMacroState &state, StreamString &stream)
+{
----------------
(nit): Please mark this function static and move it out from the anonymous
namespace
================
Comment at: source/Expression/ExpressionSourceCode.cpp:122
@@ +121,3 @@
+ else
+ return;
+ break;
----------------
Should this function return an error in this case so the caller know the macro
stream is only half ready?
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.cpp:56-58
@@ +55,5 @@
+ {
+ lldb::offset_t new_offset, str_offset;
+ uint64_t line;
+ const char *macro_str;
+
----------------
(nit): Please initialize
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.h:28
@@ +27,3 @@
+
+enum DWARFMacroHeaderFlagMask
+{
----------------
Please move this enum out from the global namespace (preferably into one of the
class) as we are currently leaking both the enum name and the enum fields into
the global namespace.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.h:38
@@ +37,3 @@
+public:
+ class Entry
+ {
----------------
(nit): struct
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.h:47
@@ +46,3 @@
+public:
+ uint8_t entry_count;
+ std::map<uint8_t, Entry> entry_map; // Mapping from opcode number to its
entry.
----------------
(nit): Member variable names should start with "m_"
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.h:48
@@ +47,3 @@
+ uint8_t entry_count;
+ std::map<uint8_t, Entry> entry_map; // Mapping from opcode number to its
entry.
+
----------------
Why do we need this data structure? Currently nobody is using it.
Also should we store DWARFFormValue instead of llvm::dwarf::Form here? It have
a lot of feature in it what makes it work with split dwarf (dwo).
http://reviews.llvm.org/D15437
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
