[PATCH] D125777: [llvm-debuginfo-analyzer] 02 - Driver and documentation

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:2
+llvm-debuginfo-analyzer - Print a logical representation of low-level debug information.
+========================================================================================
+
----------------
Is the line of `=` supposed to be exactly as long as the preceding line? It looks like there's one extra character.


================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:126
+
+ With **value** being one the options in the following lists.
+
----------------



================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:276
+
+ With **value** being one the options in the following lists.
+
----------------



================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:340
+
+ With **value** being one the options in the following lists.
+
----------------



================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:378
+
+ With **value** being one the options in the following list.
+
----------------



================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:449
+ Print all elements that satisfy the given <condition>. With **condition**
+ being one the options in the following list.
+
----------------



================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:471
+symbols (:option:`--select-symbols`) and types (:option:`--select-types`).
+These options, require knowledge of the debug information format (DWARF,
+CodeView, COFF), as the given **kind** describes a very specific type
----------------



================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:482
+
+ With **kind** being one the options in the following list.
+
----------------



================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:503
+
+ With **kind** being one the options in the following list.
+
----------------



================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:537
+
+ With **kind** being one the options in the following list.
+
----------------



================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:555
+
+ With **kind** being one the options in the following list.
+
----------------



================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:635
+When reading the input object files, :program:`llvm-debuginfo-analyzer`
+can detected issues in the raw debug information. These may not be
+considered fatal to the purpose of printing a logical view but they can
----------------



================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:645
+
+ With **value** being one the options in the following list.
+
----------------



================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:667
+ For a better understanding of the logical view, access to more detailed
+ internal information is needed. Such data would help to identify debug
+ information processed or incorrect logical element management. Typically
----------------
is -> could be (?)


================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:677
+
+ With **value** being one the options in the following list.
+
----------------



================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:711
+:program:`llvm-debuginfo-analyzer`. We compiled the example for an X86
+ELF target with a recent version of Clang (-O0 -g):
+
----------------
(two years from now it won't be "recent" anymore)


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h:43
+public:
+  // True if the scope has not been named or typed or no line number.
+  virtual bool isNamed() const { return false; }
----------------
True if it has _not_ been named?  that's inconsistent with the function name.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:49
+#define STDSET_FUNCTION_4(FAMILY, FIELD, TYPE, SET)                            \
+  bool get##FAMILY##FIELD() const {                                            \
+    return FAMILY.SET.find(TYPE::FIELD) != FAMILY.SET.end();                   \
----------------
It looks like you're using a std::set as if it were a BitVector?  Seems inefficient... although functionally equivalent.
FYI, the current trend is to use TableGen for command-line options in tools.

I won't insist you change anything at this point, but in general the TableGen approach seems to reduce the amount of hand-coding required.  You might want to investigate this later.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVStringPool.h:92
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+  virtual void dump() const { print(dbgs()); }
+#endif
----------------
Does this really need to be virtual?  If so, the destructor should also be virtual?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125777/new/

https://reviews.llvm.org/D125777