kastiglione added inline comments.
================
Comment at: lldb/examples/python/crashlog.py:434
except CrashLogFormatException:
- return TextCrashLogParser(debugger, path, verbose).parse()
+ return object().__new__(TextCrashLogParser)
----------------
mib wrote:
> JDevlieghere wrote:
> > kastiglione wrote:
> > > mib wrote:
> > > > JDevlieghere wrote:
> > > > > mib wrote:
> > > > > > kastiglione wrote:
> > > > > > > I have not seen the `object().__new__(SomeClass)` syntax. Why is
> > > > > > > it being used for `TextCrashLogParser` but not
> > > > > > > `JSONCrashLogParser`? Also, `__new__` is a static method, could
> > > > > > > it be `object.__new__(...)`? Or is there a subtly that requires
> > > > > > > an `object` instance? Somewhat related, would it be better to say
> > > > > > > `super().__new__(...)`?
> > > > > > >
> > > > > > > Also: one class construction explicitly forwards the arguments,
> > > > > > > the other does not. Is there a reason both aren't implicit (or
> > > > > > > both explicit)?
> > > > > > As you know, python class are implicitly derived from the `object`
> > > > > > type, making `object.__new__` and `super().__new__` pretty much the
> > > > > > same thing.
> > > > > >
> > > > > > In this specific case, both the `TextCrashLogParser` and
> > > > > > `JSONCrashLogParser` inherits from the `CrashLogParser` class, so
> > > > > > `JSONCrashLogParser` will just inherits `CrashLogParser.__new__`
> > > > > > implementation if we don't override it, which creates a recursive
> > > > > > loop.
> > > > > > That's why I'm calling the `__new__` method specifying the class.
> > > > > What's the advantage of this over this compared to a factory method?
> > > > > Seems like this could be:
> > > > >
> > > > > ```
> > > > > def create(debugger, path, verbose)
> > > > > try:
> > > > > return JSONCrashLogParser(debugger, path, verbose)
> > > > > except CrashLogFormatException:
> > > > > return TextCrashLogParser(debugger, path, verbose)
> > > > > ```
> > > > If we make a factory, then users could still call `__init__` on
> > > > `CrashLogParser` and create a bogus object. With this approach, they're
> > > > forced to instantiate a CrashLogParser like any another object.
> > > `CrashLogParser.__init__` could raise an exception. With intricacy of
> > > this approach, maybe it's better to use a factor method combined with an
> > > exception if the base class `__init__` is called.
> > +1, or maybe `abc` provide a capability to achieve the same?
> IMHO, having to call an arbitrary-called method (`create/make/...`) to
> instantiate an object and having the `__init__` raise an exception introduces
> more intricacies in the usage of this class, compared to what I'm doing.
>
> I prefer to keep it this way since it's more natural / safe to use. If the
> implementation exercises some python internal features, that's fine because
> that shouldn't matter to the user.
Only after discussing it with you, and reading python docs, do I understand why
this code is the way it is. Future editors, including us, could forget some
details, which isn't great for maintainability.
You mention the user, are there external users of this class hierarchy? Or are
these classes internal to crashlog.py? If the latter, then the simplified
interface seems hypothetical. If there are external users, how many are they? I
am trying to get a sense for what is gained by the avoiding a factory method.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131085/new/
https://reviews.llvm.org/D131085
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
