[PATCH] Initial code coverage mapping data, reader, writer + C interface for ProfileData

[Sean Silva]

On Thu, Jul 17, 2014 at 10:40 AM, Bob Wilson <bob.wilson at apple.com> wrote:

>
> > On Jul 9, 2014, at 4:39 PM, Alex Lorenz <arphaman at gmail.com> wrote:
> >
> > The updated version of the patch that fixes various typos, adds format
> version to the coverage mapping format, changes the storage of filenames to
> once per tu, and adds document that describes the actual coverage mapping
> format.
> >
> > http://reviews.llvm.org/D4301
> >
> > Files:
> >  docs/CoverageMappingFormat.rst
> >  docs/index.rst
> >  include/llvm/ProfileData/CoverageMapping.h
> >  include/llvm/ProfileData/CoverageMappingReader.h
> >  include/llvm/ProfileData/CoverageMappingWriter.h
> >  lib/ProfileData/CMakeLists.txt
> >  lib/ProfileData/CoverageMapping.cpp
> >  lib/ProfileData/CoverageMappingReader.cpp
> >  lib/ProfileData/CoverageMappingWriter.cpp
> >  lib/ProfileData/LLVMBuild.txt
> > <D4301.11233.patch>
>
> Here are my comments on the documentation. As I mentioned in my latest
> review of the code, I’d like to split the documentation out to a separate
> patch. I’m not going to go into details yet because this needs more work
> first.
>
> My biggest comment is about the style of the document. You’ve written it
> as a fairly formal specification, but I don’t think that is the best way to
> do it. Formal specifications are not easy to read. They’re often necessary
> if someone might have to reimplement the code from scratch, using only the
> information in the document, but that is not the case here since the code
> is freely available. The most important goal of this document should be to
> explain the format so that anyone working on the code can easily understand
> it. For that purpose, you should focus on conveying the high-level
> structure and making the document readable.
>
> I suggest you start by inverting the organization, moving the low-level
> details to the end and putting the high-level structures at the beginning.
>
> Next, the Overview section needs to be expanded to give a summary of the
> context.


Agreed. FWIW for Alex, there are some useful guidelines in
http://llvm.org/docs/SphinxQuickstartTemplate.html#guidelines

Also, include lots of examples. Lots and lots of examples.
E.g. before diving into an actual formal description, but after any
absolutely necessary conceptual preliminaries, one of the first things
should probably be a minimal example bytestream that you dissect piece by
piece to give the reader a "feel" for the format.

-- Sean Silva



> Anyone who has not delved into the details of clang’s
> instrumentation-based profiling is going to be lost otherwise. I.E., why
> are there expressions for counters instead of just simple count values? (An
> example showing how to calculate the count for the “else” block of an
> if-statement would be a good idea here.) You should also explain the
> concepts that come up later in the document, such as “expansion regions”,
> “empty regions”, and “skipped regions”.
>
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140718/6f1e7320/attachment.html>