[PATCH] D62638: [analyzer] A Python script to prettify the ExplodedGraph dumps.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Sorry for that much rewrite request but in a script language you have to name your stuff properly or you will be completely lost. I have not written a single Python class yet, but trust me.

I really like that `DotDumpVisitor` semantic, but it would be a lot better at SVG level. At the moment whatever style/table-data you insert it adds an extra text tag per styling which is quite inefficient.

We could use "foreignObject" (somehow) and apply pure HTML tables or I could use my `<tspan>` idea instead (https://developer.mozilla.org/en-US/docs/Web/SVG/Element/tspan). I recommend the latter as it is better than a fixed table.

With the current representation it would be difficult to apply every styling to tspans. I think it also would be difficult to use your `Explorer` idea as we are talking about modificating the SVG dynamically.

In D62638#1522357 <https://reviews.llvm.org/D62638#1522357>, @NoQ wrote:

> > I wrote some tests but i'm not really sure they're worth it.
>
> Mmm, on second thought, they probably won't work out of the box, because they might require installing python modules in order to work. I'm actually not sure if all machines have python3. I'll try but it'll most likely fail. I guess i could try excluding them from `make check-all`.


It would be cool to introduce something like `check-scripts`.



================
Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:5
+import collections
+import json as json_module  # 'json' is a great name for a variable.
+import logging
----------------
I think it is not that cool variable name. I would use the appropiate name of the object instead of the generic `json` name.


================
Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:12
+class SourceLocation(object):
+    def __init__(self, json):
+        super(SourceLocation, self).__init__()
----------------
`json` -> `loc`


================
Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:22
+class ProgramPoint(object):
+    def __init__(self, json):
+        super(ProgramPoint, self).__init__()
----------------
`json` -> `program_point` and so on...


================
Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:33
+            self.pretty = json['pretty']
+            self.sloc = SourceLocation(json['location']) \
+                if json['location'] is not None else None
----------------
What about just `loc`?


================
Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:124
+        self.parents = []
+        self.children = []
+
----------------
What about `predecessors` and `successors`? It is the proper name in the Static Analyzer world.


================
Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:126
+
+    def set_json(self, node_id, json):
+        logging.debug('Adding ' + node_id)
----------------
- `set_json()` -> `set()` the given `node`
- `json` -> `node`


================
Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:151
+        super(ExplodedGraph, self).__init__()
+        self.nodes = collections.defaultdict(ExplodedNode)
+        self.root_id = None
----------------
`nodes` -> `graph`


================
Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:184
+            # even though in a valid dot file everything is escaped.
+            raw_json = result[2].replace('\\l', '') \
+                                .replace(' ', '') \
----------------
`raw_json` -> `node_string` which will be a raw JSON after `loads()`.


================
Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:191
+                                .replace('\\>', '\\\\>') \
+                                .rstrip(',')
+            logging.debug(raw_json)
----------------
I have removed the trailing `,` from the end yesterday so `rstrip(',')` is not needed.

It would be cool to name the `result[]`, like: `pred_id = result[1]` and `succ_id = result[2]` or current/previous ID, or something like that.

I also would put the regexes into a function and you could write:
`pred_id, succ_id = parse_edge()` or something more simpler.

What is `result[0]` btw? That confused me a little-bit.


================
Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:233
+        else:
+            color = 'cyan3'
+
----------------
It would be cool to put it into a "switch-statement" (https://stackoverflow.com/questions/60208/replacements-for-switch-statement-in-python).


================
Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:237
+            if p.sloc is not None:
+                self._dump('<tr><td align="left" width="0">'
+                           '%s:<b>%s</b>:<b>%s</b>:</td>'
----------------
I think tables are left aligned by default, so you could remove your left alignments.


================
Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:326
+            self._dump('<b>Program point:</b></td></tr>')
+        self._dump('<tr><td align="left" balign="left" width="0">'
+                   '<table border="0" align="left" width="0">')
----------------
I would create a table-builder class to reduce the repetition and for better readability what is going on. Also you could avoid `balign` typo.

Then may each class could have its own pretty-print method like LLVM does.


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

https://reviews.llvm.org/D62638