[PATCH] D125777: [llvm-dva] 02 - Driver and documentation

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-dva.rst:19
+original user source code. Supported object file formats include ELF,
+MacOS, PDB and COFF.
+
----------------
jryans wrote:
> "MacOS" is not an object file format, so probably you mean "Mach-O" here?
You are correct. Replaced with `Mach-O`.


================
Comment at: llvm/docs/CommandGuide/llvm-dva.rst:147-148
+ user source code, where the elements are declared or defined; functions
+ with public visibility across modules. These options allow to map the
+ elements to their user code location, for cross references purposes.
+
----------------
jryans wrote:
> Nit: These sentences read a bit strangely
Replaced with your suggestion.


================
Comment at: llvm/docs/CommandGuide/llvm-dva.rst:239
+ extended knowledge about debug information. They are intended to be
+ used when a more low level detail is required.
+
----------------
psamolysov wrote:
> details?
May be `used when a more low level details are required.`


================
Comment at: llvm/docs/CommandGuide/llvm-dva.rst:510-511
+    =CatchBlock: An exception block.
+    =Class: A Class.
+    =CompileUnit: A Compile unit.
+    =EntryPoint: A subroutine entry point.
----------------
psamolysov wrote:
> Why `Class` and `Compile unit` are started from a capital letter while all the other entities (`array`, `call site`, etc.) aren't?
It should be lowercase: `class` and `compile unit`.


================
Comment at: llvm/docs/CommandGuide/llvm-dva.rst:527
+    =TryBlock: An exception try block.
+    =Union: An union.
+
----------------
psamolysov wrote:
> I'm not a language expert, but I believe there should be "A union". 
You are correct. It should be `A union.`


================
Comment at: llvm/docs/CommandGuide/llvm-dva.rst:563
+    =ImportModule: Import Module.
+    =Pointer: Pointer Type.
+    =PointerMember: Pointer to member function.
----------------
psamolysov wrote:
> `Pointer Type` is here but `Reference type` (not `Type`) is below. Also `Import Module` (not `Module`) is on the previous line.
It should be `pointer type` and `Import module`.


================
Comment at: llvm/docs/CommandGuide/llvm-dva.rst:1467
+references to the enumerators **RED** and **BLUE**. The DWARF
+generated by GCC, CodeView generated by Clang and MSVC, it does
+include such references.
----------------
psamolysov wrote:
> Two formats generated by 3 compilers.
Fixed incorrect sentence.


================
Comment at: llvm/docs/CommandGuide/llvm-dva.rst:1918
+information by each scope, which can be used to determine unexpected
+sizes changes in the DWARF sections between different versions of the
+same toolchain.
----------------
psamolysov wrote:
> 
Fixed the typo.


================
Comment at: llvm/docs/CommandGuide/llvm-dva.rst:1937
+The **{Coverage}** and **{Location}** attributes describe the debug
+location and coverage for logical symbols. For optimized code, the
+coverage value decreases and it affects the program debuggability. 
----------------
psamolysov wrote:
> To use the same order for the words.
Goot catch. Changed.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:193
+// members that in very few cases point to a command option (see associated
+// comment. Other cases for 'bool' refers to internal values derivated from
+// the command options.
----------------
psamolysov wrote:
> the closed bracket is missed.
Added closed bracket.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:220
+    bool Formatting = true;    // Disable formatting during printing.
+    bool Offset = false;       // Print offsets while formatting is disable.
+    bool SizesSummary = false; // Print 'sizes' or 'summary'.
----------------
psamolysov wrote:
> 
Fixed typo.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:236
+    bool Execute = false;        // Select requested.
+    bool GenericKind = false;    // We have collected generic kind.
+    bool GenericPattern = false; // We have collected generic patterns.
----------------
psamolysov wrote:
> kinds or there will be a single kind only? There are a bunch of kind sets below, though.
It should be `kinds`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125777