[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 #63 from Dmitry Selyutin <ghostmansd at gmail.com> ---
(In reply to Luke Kenneth Casson Leighton from comment #61)
> (In reply to Dmitry Selyutin from comment #60)
> 
> 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 _again_ skip the rationale I posted. I'm worrying this became a _fixed_
pattern. I already told the rationale why there's a property, please finally
_read_ this. TL;DR: this is too low-level yet to be used in dataclasses.fields.
 If you have budget for adopting insndb for higher-level — fine, I'm OK to do
it. Meanwhile even this task doesn't have the budget allocated.

> 
> you're already using dataclasses.fields():

Yes I do, because I know the low-level details of insndb. The users of visitors
shouldn't.

> 
>   
> 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)

No it isn't for many scenarios, other than restricting the inputs. Almost any
scenario can be solved either by pattern matching or polymorphism. Please don't
break duck typing unless really unavoidable.

> *, then using "hasattr()" to detect

No need for hasattr. Again: duck typing and polymorphism. You already have Node
class. If all fields of insndb have this type, they are traversible for free.
Refer to the above.

> 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.

That's basically the same thing inverted. "Propely-named" methods are no way
better than isinstance in this regard.

> > 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).

Yes, one can walk db, collect all relevant entries, then visit these. That's be
better.

> 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

There's no difference. You can affect visitor's call either way.

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