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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
