[PATCH] Code coverage tool that uses the new coverage mapping format and clang's instrumentation based profiling data

[Alex L]

2014-08-21 12:44 GMT-07:00 Duncan P. N. Exon Smith <dexonsmith at apple.com>:

>
> > On 2014-Aug-21, at 12:21, Alex Lorenz <arphaman at gmail.com> wrote:
> >
> > Thanks for the suggestions, I have updated the patch and have some
> comments:
> >
> >    This pattern shows up a few times. It might be nicer to make an RAII
> >    object for it and write something like:
> >
> >      {
> >        ColoredOutputRAII C(ViewOpts.Colors, outs(), raw_ostream::CYAN);
> >        outs() << Function.PrettyName << " from " << SourceFile << ":";
> >      }
> >
> >     WDYT?
> >
> > I've implemented something similar to this in the updated patch, please
> take a look.
> >
> >   I wonder if we really need both Regions and SortedRegions in this
> >   structure. It looks like regions is just used to unique the input, and
> >   then when we want to use it, we sort first.
> >
> >   How many duplicate entries do we expect? Would it make sense to add
> >   items directly to the vector, and then std::unique it?
> >
> > std::unique isn't really suitable as we have to sum the execution counts
> somehow. The number of duplicate entries depends on the particular source
> code - the use of macros and templates will increase this.
>
> `std::unique` is simple to write and customize.  IIUC, your flow is to
> gather and coalesce the regions, then sort them (for profit?).  You can
> change this to gather, then sort and coalesce.  `std::unordered_map` is
> a pretty heavy tool unless you actually need its iterator guarantees.
>

Of course, this is so much better, why didn't I think about this before ?
Thanks


>
> This is how you can sort and coalesce:
>
>     if (Regions.size() <= 1)
>       return;
>     std::sort(Regions.begin(), Regions.end());
>     auto O = Regions.begin();
>     for (auto I = O + 1, E = Regions.end(); I != E; ++I)
>       if (I->first == O->first)
>         O->second += I->second;
>       else
>         *++O = std::move(*I);
>     Regions.erase(++O, Regions.end());
>
> >    Any reason this can't be a DenseMap?
> >
> > It's simpler to use unordered_map here as DenseMap needs DenseMapInfo.
>
> You've done most of the work already by writing a hash function.
> `unordered_map<>` is a heavy tool, so you should prefer `DenseMap<>`
> unless you need its guarantees.
>
> >
> > I've also removed the demangling support as it's something that can be
> committed in a later patch.
> >
> > http://reviews.llvm.org/D4445
> >
> > Files:
> >  tools/llvm-cov/CMakeLists.txt
> >  tools/llvm-cov/CodeCoverage.cpp
> >  tools/llvm-cov/CoverageFilters.cpp
> >  tools/llvm-cov/CoverageFilters.h
> >  tools/llvm-cov/CoverageReport.cpp
> >  tools/llvm-cov/CoverageReport.h
> >  tools/llvm-cov/CoverageSummary.cpp
> >  tools/llvm-cov/CoverageSummary.h
> >  tools/llvm-cov/CoverageSummaryInfo.cpp
> >  tools/llvm-cov/CoverageSummaryInfo.h
> >  tools/llvm-cov/CoverageViewOptions.h
> >  tools/llvm-cov/FunctionCoverageMapping.h
> >  tools/llvm-cov/LLVMBuild.txt
> >  tools/llvm-cov/Makefile
> >  tools/llvm-cov/RenderingSupport.h
> >  tools/llvm-cov/SourceCoverageDataManager.cpp
> >  tools/llvm-cov/SourceCoverageDataManager.h
> >  tools/llvm-cov/SourceCoverageView.cpp
> >  tools/llvm-cov/SourceCoverageView.h
> >  tools/llvm-cov/llvm-cov.cpp
> > <D4445.12790.patch>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140821/bb83a668/attachment.html>