[PATCH] D23277: Improves the HTML report for the source file.

[Vedant Kumar via llvm-commits]

vsk added a comment.

On a high-level, the changes you've made look good. However, I think that this
patch needs to be split up into 3 smaller patches for each functional change
you listed in your summary. That would make it easier for me to review it, and
would make the commit history easier to read through.

I've included a few inline comments which might help you split the code up.


================
Comment at: tools/llvm-cov/SourceCoverageView.h:262
@@ +261,3 @@
+  /// \brief Render the project summary for a given source file name.
+  virtual void renderProjectSummary(raw_ostream &OS) = 0;
+
----------------
The two implementations of renderProjectSummary() are very similar. Could you pull the code into SourceCoverageView, and modify renderViewHeader() to account for any differences (e.g, by passing `bool WholeFile` into it)?

Once this is done, please go through the pure virtual methods you've added and eliminate the ones which are not directly called by the SourceCoverageView class.

================
Comment at: tools/llvm-cov/SourceCoverageView.h:268
@@ +267,3 @@
+  /// \brief Render the javascript header for a given source file.
+  virtual void renderJavaScriptHeader(raw_ostream &OS) = 0;
+
----------------
I don't think it makes sense to have a method called 'renderJavaScriptHeader' in generic presentation code. It would make more sense to me to sink this logic into another method, e.g renderViewFooter().

================
Comment at: tools/llvm-cov/SourceCoverageViewHTML.cpp:362
@@ +361,3 @@
+  std::string SourceFile = getSourceName().str();
+  SourceFile = sys::path::has_extension(SourceFile)
+                   ? "Source file:        " + SourceFile
----------------
I'd rather not rely on file extension naming conventions on getting this right. Could you modify createFunctionView() to sneak an indicator bit into SourceCoverageView?


https://reviews.llvm.org/D23277