[PATCH] D131518: Make opt-viewer more usable by general developers - part 1/N

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D131518#3721299 <https://reviews.llvm.org/D131518#3721299>, @OfekShilon wrote:

> @fhahn  You're right of course. I trimmed this patch to include just the added example and the two-line python requirements file.  Hopefully this would be an easy approval and I'll add more gradual patches to be reviewed.
>
> Regarding documentation, 3 directions come to mind:
>
> 1. Since llvm is on now github, a README.md file can be used. I have a decent starting point ready here <https://github.com/OfekShilon/optview2/blob/main/README.md> (after renaming from "OptView2" of course) and this might be the easiest.
> 2. Alternatively, this content can be spilled into ./llvm/docs/Remarks.rst - extending the existing opt-viewer section <https://llvm.org/docs/Remarks.html#opt-viewer>
> 3. Finally I can create a separate ./llvm/docs/OptViewer.rst and link to it from Remarks.rst.
>
> This will probably be the last patch I submit anyway. What do you think?

Sounds good to me. I think we should probably have a readme and it might be worth to split off the existing opt-viewer section and add a separate `.rst` for it.



================
Comment at: llvm/tools/opt-viewer/cpp_optimization_example/Makefile:1
+CC=clang++
+CXX=clang++
----------------
Not sure if there's an easy way to do it, but if possible it would be good to default to custom build of clang, if that's enabled. Not sure fi that's easily possible though.


================
Comment at: llvm/tools/opt-viewer/cpp_optimization_example/main.cc:1
+#include <iostream>
+#include <vector>
----------------
It would probably be good to add the llvm license header here


================
Comment at: llvm/tools/opt-viewer/cpp_optimization_example/main.cc:5
+// Based on Roi Barkan - "Argument passing, core guidelines and concepts" - https://www.youtube.com/watch?v=uylFACqcWYI
+void scale_down(std::vector<double>& v, const double& a) {
+    for (auto& item : v) {
----------------
It would probably be good to format this with clang-format and the LLVM code style.


================
Comment at: llvm/tools/opt-viewer/cpp_optimization_example/main.cc:14
+    scale_down(v, v[0]);
+
+}
----------------
nit: stray new line?


================
Comment at: llvm/tools/opt-viewer/cpp_optimization_example/run_optview.sh:2
+#!/bin/bash
+set -euo pipefail
+cd "$(dirname "$0")" || exit 1
----------------
Should this also create a virtual-env and install dependencies?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131518/new/

https://reviews.llvm.org/D131518