<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">2014-08-21 12:44 GMT-07:00 Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class=""><br>
> On 2014-Aug-21, at 12:21, Alex Lorenz <<a href="mailto:arphaman@gmail.com">arphaman@gmail.com</a>> wrote:<br>
><br>
> Thanks for the suggestions, I have updated the patch and have some comments:<br>
><br>
>    This pattern shows up a few times. It might be nicer to make an RAII<br>
>    object for it and write something like:<br>
><br>
>      {<br>
>        ColoredOutputRAII C(ViewOpts.Colors, outs(), raw_ostream::CYAN);<br>
>        outs() << Function.PrettyName << " from " << SourceFile << ":";<br>
>      }<br>
><br>
>     WDYT?<br>
><br>
> I've implemented something similar to this in the updated patch, please take a look.<br>
><br>
>   I wonder if we really need both Regions and SortedRegions in this<br>
>   structure. It looks like regions is just used to unique the input, and<br>
>   then when we want to use it, we sort first.<br>
><br>
>   How many duplicate entries do we expect? Would it make sense to add<br>
>   items directly to the vector, and then std::unique it?<br>
><br>
> 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.<br>
<br>
</div>`std::unique` is simple to write and customize.  IIUC, your flow is to<br>
gather and coalesce the regions, then sort them (for profit?).  You can<br>
change this to gather, then sort and coalesce.  `std::unordered_map` is<br>
a pretty heavy tool unless you actually need its iterator guarantees.<br></blockquote><div><br></div><div>Of course, this is so much better, why didn't I think about this before ?</div><div>Thanks</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

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