[PATCH] Coverage: Documentation for the coverage mapping format

[Sean Silva]

================
Comment at: docs/CoverageMappingFormat.rst:14-16
@@ +13,5 @@
+
+This document describes the LLVM code coverage mapping format,
+the way the coverage mapping data is stored in the LLVM IR,
+and the way the coverage mapping data is encoded.
+
----------------
Abstracts don't work well for most kinds of documentation (this case included). It's much more important to specify your assumptions about your readers and what the document is *trying to achieve for those readers*, than just trying to summarize the document's contents per se. See http://llvm.org/docs/SphinxQuickstartTemplate.html#guidelines for some basic guidelines.

================
Comment at: docs/CoverageMappingFormat.rst:43-44
@@ +42,4 @@
+
+Mapping Region
+--------------
+
----------------
A reader could make it here (and actually beyond here) thinking that this format is the run-time data structure that is used to store counts; I speak from personal experience...

Please don't address this comment by just saying somewhere "this is not a discussion of the runtime data structure". Instead, describe where this information fits in the coverage pipeline (i.e. the entire process of going from "a user's source code" to "a user's source code displayed to them with coverage information annotated on it"), which programs handle/create/look at this information, what does "mapping" have to do with anything?, etc. This is key "orienting" information that the reader absolutely has to have clear for the document to make any sense at all!

Obviously, it can be deduced from various aspects of this document what role this information must play. But the reader shouldn't have to play Sherlock. Actually, I can only find three clues that what this document describes is *not* the runtime data structure: "stored in object files and IR", "produced by frontend", "stores its integers in a format not suitable for random-access".

================
Comment at: docs/CoverageMappingFormat.rst:56
@@ +55,3 @@
+
+  :raw-html:`<pre class='highlight' style='line-height:initial;'><span>int main(int argc, const char *argv[]) </span><span style='background-color:#4A789C'>{   </span><span> </span><span class='c1'>// Code Region from 1:40 to 9:2</span>
+  <span style='background-color:#4A789C'>                                           </span><span> </span>
----------------
Is there a reason you didn't use a `<pre>` here? These non-breaking space entities make it basically impossible to edit this manually or review it in source form.

================
Comment at: docs/CoverageMappingFormat.rst:189
@@ +188,3 @@
+   [48 x i8] c"..." ; Encoded data (not shown)
+  }, section "__DATA,__llvm_covmap", align 8
+
----------------
This is Darwin-specific. If this has to be included please accompany it with a discussion of portability and what would be different for other platforms.

================
Comment at: docs/CoverageMappingFormat.rst:224-225
@@ +223,4 @@
+
+Encoding
+========
+
----------------
Before diving into the description of the format, please display (in a suitable format) and do a step-by-step dissection/walkthrough of the contents of the field `[48 x i8] c"..." ; Encoded data (not shown)` that you ellipsis'd above, so that the reader can "get a feel for it".

================
Comment at: docs/CoverageMappingFormat.rst:238-239
@@ +237,4 @@
+
+Primitives
+----------
+
----------------
It doesn't really make sense to talk about "primitives" at all, since your strings aren't even "primitive" (they contain a LEB128, which is itself a primitive).

Better to instead have a section about "the things that can appear after the : in [foo : type]". (see my comment below about simplifying the use of subscripts to use a simpler colon notation)

================
Comment at: docs/CoverageMappingFormat.rst:257
@@ +256,3 @@
+:raw-html:`<tt>`
+[length\ :sub:`uint`, characters...]
+:raw-html:`</tt>`
----------------
Everywhere that you currently say `uint` just say `LEB128`. That eliminates the need for the reader to look around at what you mean by `uint` (readers do not always read sequentially). It also saves you from having to explain what a "uint" is in the first place besides simply linking to a description of LEB128.

================
Comment at: docs/CoverageMappingFormat.rst:278-280
@@ +277,5 @@
+
+:raw-html:`<tt>`
+[value\ :sub:`uint`]
+:raw-html:`</tt>`
+
----------------
This notation seems more fit for a LaTeX document, but as you can tell it's a bit of a bear in Sphinx. Please use a simpler, flat notation for this. Just [value : uint] would be fine, placed in monospace like you have done for the section "Mapping Region"

================
Comment at: docs/CoverageMappingFormat.rst:332-333
@@ +331,4 @@
+
+Counter expressions consist of two counters as they
+represent binary arithmetic operations.
+
----------------
How do you know which operation to apply? There seems to be a LHS and a RHS but no "opcode".

http://reviews.llvm.org/D4729