[PATCH] D111042: flang-omp-report summarising script

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added inline comments.


================
Comment at: flang/examples/flang-omp-report-plugin/yaml_summarizer.py:3-4
+
+This Python script generates a YAML summary from
+the files generated by ``flang-omp-report``.
+Currently, it only support ``ruamel.yaml``,
----------------
[nit] Could you add a note explaining why one can't simply use the plugin here? 


================
Comment at: flang/examples/flang-omp-report-plugin/yaml_summarizer.py:5
+the files generated by ``flang-omp-report``.
+Currently, it only support ``ruamel.yaml``,
+which can be installed with:
----------------



================
Comment at: flang/examples/flang-omp-report-plugin/yaml_summarizer.py:20
+
+    -l   --log         This option combines all yaml files into one
+
----------------
IMHO, you can safely remove `This option ` (it is clear that it's an option ;-) ). On the other hand, it would be helpful to emphasis the difference between `-l` and the default behavior. See my suggestion inline.


================
Comment at: flang/examples/flang-omp-report-plugin/yaml_summarizer.py:41
+
+def find_yaml_files(search_pattern):
+    """
----------------
You don't need the search pattern outside this method. And to me, `find_yaml_files` should only care about the directory in which to search and whether to do secretively. The search pattern itself that you pass to `glob.iglob` is just an implementation detail.

But your current approach is correct and this comment is just a nit.


================
Comment at: flang/examples/flang-omp-report-plugin/yaml_summarizer.py:50-54
+    # @TODO: Currently yaml files not from the 'flang-omp-report' plugin could be
+    # included resulting in errors. So we need to check whether the yaml files are
+    # generated in the build or source directory. If in build directory,
+    # look at adding some check for a prefix in yaml file name. If in source, add a
+    # check for the yaml file against its corresponding source file.
----------------
This an important comment, but provides a solution a bit preemptively. And, by differentiating between the source and build dirs, it's become rather long. Also, I'm concerned that it might be quite hard to find a good solution and perhaps not worth it?

 How about this:
```
   # @TODO: Currently *all* yaml files are read - regardless of whether they have been generated with  'flang-omp-report' or not. This might result in the script reading files that it should ignore.
```


================
Comment at: flang/examples/flang-omp-report-plugin/yaml_summarizer.py:69-80
+    Example after being processed to yaml:
+        /../llvm-project/flang/test/Examples/omp-device-constructs.f90:
+        - construct: target
+          line: 18
+          clauses:
+          - clause: map
+              details: arraya
----------------
Wouldn't the output with `-l` only really differ when at least 2 input files were used?


================
Comment at: flang/examples/flang-omp-report-plugin/yaml_summarizer.py:95
+    Keyword arguments:
+    dataum -- Data construct containing clauses to check.
+    construct -- Construct to add or increment clause count.
----------------



================
Comment at: flang/examples/flang-omp-report-plugin/yaml_summarizer.py:118-135
+    Example after being processed to yaml:
+        - construct: target data
+          num: 1
+          clauses:
+          - clause: device
+              num: 1
+          - clause: map
----------------
Was this generated using `/../llvm-project/flang/test/Examples/omp-device-constructs.f90`? (like for `process_log`?)


================
Comment at: flang/examples/flang-omp-report-plugin/yaml_summarizer.py:162
+    output_file -- File to output result to. If this is 'None' then result will be
+                  outputted to 'stdout'. (Default: None)
+    """
----------------
[nit] That's clear from the function definition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111042