[Libre-soc-bugs] [Bug 1094] insndb instruction database visitor-walker is needed

[bugzilla-daemon at libre-soc.org]

https://bugs.libre-soc.org/show_bug.cgi?id=1094

--- Comment #61 from Luke Kenneth Casson Leighton <lkcl at lkcl.net> ---
(In reply to Dmitry Selyutin from comment #60)
> Decoupled visitors from walking:
> 
> https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;
> h=7de83fc89028c55a36af2210b80c426e24e6b5b9
> 
> The record filtering is now implemented in terms of concrete visitor. That's
> the whole code:
> 
> 
> class Visitor:
>     def __call__(self, node):
>         method = node.__class__.__name__
>         method = getattr(self, method, self.Node)
>         return method(node=node)
> 
>     @_contextlib.contextmanager
>     def Node(self, node):
>         for subnode in node.subnodes:
>             with self(subnode):
>                 pass
>         yield node
> 
> class Record(Node):
>     @property
>     def subnodes(self):
>         for (name, fields) in self.extras.items():
>             yield Extra(name=name, **fields)
> 
> class Database(Node):
>     @property
>     def subnodes(self):
>         yield from self
> 
> 
> Obviously there's more in Record and Database to be visited, just a
> practical example.

that's a *massive* amount of work, adding for-loops on every single
object on every single class.   all of it unnecessary.
here's the thing: they *all* can be done by introspection of the
fields.

you're already using dataclasses.fields():

  
https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/insndb/core.py;h=2876e787#l127

in ast.py simply replace "for field in node._fields" with
                         "for field in dataclasses.fields(self)"

   https://github.com/python/cpython/blob/2.7/Lib/ast.py#L166

also in iter_child_nodes, "if isinstance(field, AST):"
replace with              "if isinstance(field, Node)

   https://github.com/python/cpython/blob/2.7/Lib/ast.py#L179

then make sure that *all* InsnDb classes inherit from Node.
a possible exception is Instruction because it inherits from
str.


you're either going to need to bite the bullet on use of "isinstance",
(please don't consider it "crap", it is pretty normal in this kind
of code, and in python in general), or come up with an awful-hack
of having a hidden identifier function/property of Node that is unique
and is *not* present in str, list, tuple, bool, int, or any other
base datatype *or class instance*, then using "hasattr()" to detect
it.

and that will violate the "Principle of Least Surprise", it is
using a non-standard pattern for a *known* expected pattern
(isinstance).  it will become a maintenance and usage nightmare.


> FWIW, here's how the filtering now looks:
> 
> class RecordNameVisitor(Visitor):
>     def __init__(self, name):
>         self.__name = name
>         self.__records = set()
>         return super().__init__()
> 
>     @contextlib.contextmanager
>     def Record(self, node):
>         if node.name == self.__name:
>             self.__records.add(node)
>         yield node
> 
>     def __iter__(self):
>         yield from self.__records

ok so this is storing all the records then replaying them?
that's not necessary to do because the database can be
considered static (it's not going to change).

also it can be replaced with a single-line lambda function for
a fixed match:

   matcher = lambda node: node.name == "XO"

and for one that is more generic:

class RecordMatcher: # no derivation from any other class
   def __init__(self, name): self.name = name
   def __call__(self, node): return self.name == node.name

   matcher = RecordMatcher("XO")

that's it.  that's all that's needed. it should not be performing
any "walking" itself because it is passed *to* the walker-function as
an (optional) parameter.

-- 
You are receiving this mail because:
You are on the CC list for the bug.