[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG66f8819c5087: [lldb/crashlog] Refactor the CrashLogParser 
logic (authored by mib).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131085

Files:
  lldb/examples/python/crashlog.py
  lldb/examples/python/scripted_process/crashlog_scripted_process.py

Index: lldb/examples/python/scripted_process/crashlog_scripted_process.py
===
--- lldb/examples/python/scripted_process/crashlog_scripted_process.py
+++ lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -10,10 +10,8 @@
 
 class CrashLogScriptedProcess(ScriptedProcess):
 def parse_crashlog(self):
-try:
-crash_log = CrashLogParser().parse(self.dbg, self.crashlog_path, False)
-except Exception as e:
-raise e
+crashlog_parser = CrashLogParser(self.dbg, self.crashlog_path, False)
+crash_log = crashlog_parser.parse()
 
 self.pid = crash_log.process_id
 self.addr_mask = crash_log.addr_mask
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -26,6 +26,7 @@
 #   PYTHONPATH=/path/to/LLDB.framework/Resources/Python ./crashlog.py ~/Library/Logs/DiagnosticReports/a.crash
 #--
 
+import abc
 import concurrent.futures
 import contextlib
 import datetime
@@ -415,37 +416,46 @@
 pass
 
 class CrashLogParser:
-def parse(self, debugger, path, verbose):
-try:
-return JSONCrashLogParser(debugger, path, verbose).parse()
-except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
-
+"CrashLog parser base class and factory."
+def __new__(cls, debugger, path, verbose):
+data = JSONCrashLogParser.is_valid_json(path)
+if data:
+self = object.__new__(JSONCrashLogParser)
+self.data = data
+return self
+else:
+return object.__new__(TextCrashLogParser)
 
-class JSONCrashLogParser:
 def __init__(self, debugger, path, verbose):
 self.path = os.path.expanduser(path)
 self.verbose = verbose
 self.crashlog = CrashLog(debugger, self.path, self.verbose)
 
-def parse_json(self, buffer):
-try:
-return json.loads(buffer)
-except:
-# The first line can contain meta data. Try stripping it and try
-# again.
-head, _, tail = buffer.partition('\n')
-return json.loads(tail)
-
+@abc.abstractmethod
 def parse(self):
-with open(self.path, 'r') as f:
-buffer = f.read()
+pass
+
 
+class JSONCrashLogParser(CrashLogParser):
+@staticmethod
+def is_valid_json(path):
+def parse_json(buffer):
+try:
+return json.loads(buffer)
+except:
+# The first line can contain meta data. Try stripping it and
+# try again.
+head, _, tail = buffer.partition('\n')
+return json.loads(tail)
+
+with open(path, 'r') as f:
+buffer = f.read()
 try:
-self.data = self.parse_json(buffer)
+return parse_json(buffer)
 except:
-raise CrashLogFormatException()
+return None
 
+def parse(self):
 try:
 self.parse_process_info(self.data)
 self.parse_images(self.data['usedImages'])
@@ -592,7 +602,7 @@
 INSTRS = 5
 
 
-class TextCrashLogParser:
+class TextCrashLogParser(CrashLogParser):
 parent_process_regex = re.compile('^Parent Process:\s*(.*)\[(\d+)\]')
 thread_state_regex = re.compile('^Thread ([0-9]+) crashed with')
 thread_instrs_regex = re.compile('^Thread ([0-9]+) instruction stream')
@@ -615,13 +625,10 @@
   r'(/.*)'   # img_path
  )
 
-
 def __init__(self, debugger, path, verbose):
-self.path = os.path.expanduser(path)
-self.verbose = verbose
+super().__init__(debugger, path, verbose)
 self.thread = None
 self.app_specific_backtrace = False
-self.crashlog = CrashLog(debugger, self.path, self.verbose)
 self.parse_mode = CrashLogParseMode.NORMAL
 self.parsers = {
 CrashLogParseMode.NORMAL : self.parse_normal,
@@ -1012,7 +1019,7 @@
 if not os.path.exists(crashlog_path):
 raise InteractiveCrashLogException("crashlog file %s does not exist" % crashlog_path)
 
-crashlog = CrashLogParser().parse(debugger, crashlog_path, False)
+

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

In D131085#3711104 , @kastiglione 
wrote:

> thanks for having patience with us reviewers, lgtm with one comment about 
> python versions

Thanks for the review :) !


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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 451306.
mib added a comment.

Remove walrus (`:=`) operator


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

https://reviews.llvm.org/D131085

Files:
  lldb/examples/python/crashlog.py
  lldb/examples/python/scripted_process/crashlog_scripted_process.py

Index: lldb/examples/python/scripted_process/crashlog_scripted_process.py
===
--- lldb/examples/python/scripted_process/crashlog_scripted_process.py
+++ lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -10,10 +10,8 @@
 
 class CrashLogScriptedProcess(ScriptedProcess):
 def parse_crashlog(self):
-try:
-crash_log = CrashLogParser().parse(self.dbg, self.crashlog_path, False)
-except Exception as e:
-raise e
+crashlog_parser = CrashLogParser(self.dbg, self.crashlog_path, False)
+crash_log = crashlog_parser.parse()
 
 self.pid = crash_log.process_id
 self.addr_mask = crash_log.addr_mask
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -26,6 +26,7 @@
 #   PYTHONPATH=/path/to/LLDB.framework/Resources/Python ./crashlog.py ~/Library/Logs/DiagnosticReports/a.crash
 #--
 
+import abc
 import concurrent.futures
 import contextlib
 import datetime
@@ -415,37 +416,46 @@
 pass
 
 class CrashLogParser:
-def parse(self, debugger, path, verbose):
-try:
-return JSONCrashLogParser(debugger, path, verbose).parse()
-except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
-
+"CrashLog parser base class and factory."
+def __new__(cls, debugger, path, verbose):
+data = JSONCrashLogParser.is_valid_json(path)
+if data:
+self = object.__new__(JSONCrashLogParser)
+self.data = data
+return self
+else:
+return object.__new__(TextCrashLogParser)
 
-class JSONCrashLogParser:
 def __init__(self, debugger, path, verbose):
 self.path = os.path.expanduser(path)
 self.verbose = verbose
 self.crashlog = CrashLog(debugger, self.path, self.verbose)
 
-def parse_json(self, buffer):
-try:
-return json.loads(buffer)
-except:
-# The first line can contain meta data. Try stripping it and try
-# again.
-head, _, tail = buffer.partition('\n')
-return json.loads(tail)
-
+@abc.abstractmethod
 def parse(self):
-with open(self.path, 'r') as f:
-buffer = f.read()
+pass
+
 
+class JSONCrashLogParser(CrashLogParser):
+@staticmethod
+def is_valid_json(path):
+def parse_json(buffer):
+try:
+return json.loads(buffer)
+except:
+# The first line can contain meta data. Try stripping it and
+# try again.
+head, _, tail = buffer.partition('\n')
+return json.loads(tail)
+
+with open(path, 'r') as f:
+buffer = f.read()
 try:
-self.data = self.parse_json(buffer)
+return parse_json(buffer)
 except:
-raise CrashLogFormatException()
+return None
 
+def parse(self):
 try:
 self.parse_process_info(self.data)
 self.parse_images(self.data['usedImages'])
@@ -592,7 +602,7 @@
 INSTRS = 5
 
 
-class TextCrashLogParser:
+class TextCrashLogParser(CrashLogParser):
 parent_process_regex = re.compile('^Parent Process:\s*(.*)\[(\d+)\]')
 thread_state_regex = re.compile('^Thread ([0-9]+) crashed with')
 thread_instrs_regex = re.compile('^Thread ([0-9]+) instruction stream')
@@ -615,13 +625,10 @@
   r'(/.*)'   # img_path
  )
 
-
 def __init__(self, debugger, path, verbose):
-self.path = os.path.expanduser(path)
-self.verbose = verbose
+super().__init__(debugger, path, verbose)
 self.thread = None
 self.app_specific_backtrace = False
-self.crashlog = CrashLog(debugger, self.path, self.verbose)
 self.parse_mode = CrashLogParseMode.NORMAL
 self.parsers = {
 CrashLogParseMode.NORMAL : self.parse_normal,
@@ -1012,7 +1019,7 @@
 if not os.path.exists(crashlog_path):
 raise InteractiveCrashLogException("crashlog file %s does not exist" % crashlog_path)
 
-crashlog = CrashLogParser().parse(debugger, crashlog_path, False)
+crashlog = CrashLogParser(debugger, crashlog_path, False).parse()
 
 target = lldb.SBTarget()
 # 1. Try to use the user-provided target
@@ -1257,7 +1264,7 @@
 

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Dave Lee via Phabricator via lldb-commits]

kastiglione accepted this revision.
kastiglione added a comment.
This revision is now accepted and ready to land.

thanks for having patience with us reviewers, lgtm with one comment about 
python versions




Comment at: lldb/examples/python/crashlog.py:421
+def __new__(cls, debugger, path, verbose):
+if data := JSONCrashLogParser.is_valid_json(path):
+self = object.__new__(JSONCrashLogParser)

the walrus operator (`:=`) is python 3.8 or newer, just want to make sure we 
don't expect this code to run in 3.6 or other earlier versions


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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 451266.
mib added a comment.

Implement @kastiglione suggestion


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

https://reviews.llvm.org/D131085

Files:
  lldb/examples/python/crashlog.py
  lldb/examples/python/scripted_process/crashlog_scripted_process.py

Index: lldb/examples/python/scripted_process/crashlog_scripted_process.py
===
--- lldb/examples/python/scripted_process/crashlog_scripted_process.py
+++ lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -10,10 +10,8 @@
 
 class CrashLogScriptedProcess(ScriptedProcess):
 def parse_crashlog(self):
-try:
-crash_log = CrashLogParser().parse(self.dbg, self.crashlog_path, False)
-except Exception as e:
-raise e
+crashlog_parser = CrashLogParser(self.dbg, self.crashlog_path, False)
+crash_log = crashlog_parser.parse()
 
 self.pid = crash_log.process_id
 self.addr_mask = crash_log.addr_mask
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -26,6 +26,7 @@
 #   PYTHONPATH=/path/to/LLDB.framework/Resources/Python ./crashlog.py ~/Library/Logs/DiagnosticReports/a.crash
 #--
 
+import abc
 import concurrent.futures
 import contextlib
 import datetime
@@ -415,37 +416,45 @@
 pass
 
 class CrashLogParser:
-def parse(self, debugger, path, verbose):
-try:
-return JSONCrashLogParser(debugger, path, verbose).parse()
-except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
-
+"CrashLog parser base class and factory."
+def __new__(cls, debugger, path, verbose):
+if data := JSONCrashLogParser.is_valid_json(path):
+self = object.__new__(JSONCrashLogParser)
+self.data = data
+return self
+else:
+return object.__new__(TextCrashLogParser)
 
-class JSONCrashLogParser:
 def __init__(self, debugger, path, verbose):
 self.path = os.path.expanduser(path)
 self.verbose = verbose
 self.crashlog = CrashLog(debugger, self.path, self.verbose)
 
-def parse_json(self, buffer):
-try:
-return json.loads(buffer)
-except:
-# The first line can contain meta data. Try stripping it and try
-# again.
-head, _, tail = buffer.partition('\n')
-return json.loads(tail)
-
+@abc.abstractmethod
 def parse(self):
-with open(self.path, 'r') as f:
-buffer = f.read()
+pass
+
 
+class JSONCrashLogParser(CrashLogParser):
+@staticmethod
+def is_valid_json(path):
+def parse_json(buffer):
+try:
+return json.loads(buffer)
+except:
+# The first line can contain meta data. Try stripping it and
+# try again.
+head, _, tail = buffer.partition('\n')
+return json.loads(tail)
+
+with open(path, 'r') as f:
+buffer = f.read()
 try:
-self.data = self.parse_json(buffer)
+return parse_json(buffer)
 except:
-raise CrashLogFormatException()
+return None
 
+def parse(self):
 try:
 self.parse_process_info(self.data)
 self.parse_images(self.data['usedImages'])
@@ -592,7 +601,7 @@
 INSTRS = 5
 
 
-class TextCrashLogParser:
+class TextCrashLogParser(CrashLogParser):
 parent_process_regex = re.compile('^Parent Process:\s*(.*)\[(\d+)\]')
 thread_state_regex = re.compile('^Thread ([0-9]+) crashed with')
 thread_instrs_regex = re.compile('^Thread ([0-9]+) instruction stream')
@@ -615,13 +624,10 @@
   r'(/.*)'   # img_path
  )
 
-
 def __init__(self, debugger, path, verbose):
-self.path = os.path.expanduser(path)
-self.verbose = verbose
+super().__init__(debugger, path, verbose)
 self.thread = None
 self.app_specific_backtrace = False
-self.crashlog = CrashLog(debugger, self.path, self.verbose)
 self.parse_mode = CrashLogParseMode.NORMAL
 self.parsers = {
 CrashLogParseMode.NORMAL : self.parse_normal,
@@ -1012,7 +1018,7 @@
 if not os.path.exists(crashlog_path):
 raise InteractiveCrashLogException("crashlog file %s does not exist" % crashlog_path)
 
-crashlog = CrashLogParser().parse(debugger, crashlog_path, False)
+crashlog = CrashLogParser(debugger, crashlog_path, False).parse()
 
 target = lldb.SBTarget()
 # 1. Try to use the user-provided target
@@ -1257,7 +1263,7 @@
 except 

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/examples/python/crashlog.py:434
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return  object().__new__(TextCrashLogParser)
 

kastiglione wrote:
> kastiglione wrote:
> > 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.
> here's an idea that may simplify things:
> 
> instead of embedding validation inside `JSONCrashLogParser.__new__`, have a 
> static/class method for validation. Then, the base class can decide which 
> subclass by calling the validation method. This means the subclasses don't 
> need to override `__new__`. Ex:
> 
> ```
> class Base:
>   def __new__(cls, i: int):
> if Sub1.is_valid(i):
>   return object.__new__(Sub1)
> else:
>   return object.__new__(Sub2)
> 
>   def __init__(self, i: int):
> print("Base", i)
> 
> class Sub1(Base):
>   @staticmethod
>   def is_valid(i: int):
> return i == 1234
> 
>   def __init__(self, i: int):
> super().__init__(i)
> print("Sub1", i)
> 
> class Sub2(Base):
>   def __init__(self, i: int):
> super().__init__(i)
> print("Sub2", i)
> ```
@JDevlieghere might be able to give more details on our users but `crashlog.py` 
could be imported in other python script, and from what he told me, we have 
some internal users that rely on it.

I don't have any strong opinion wrt your suggestion, I'll try it out and update 
the patch if that works well. Thanks @kastiglione !

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

In D131085#3710597 , @kastiglione 
wrote:

> was the chmod intentional?

nop, thanks for catching that


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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

was the chmod intentional?


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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/examples/python/crashlog.py:434
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return  object().__new__(TextCrashLogParser)
 

kastiglione wrote:
> 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.
here's an idea that may simplify things:

instead of embedding validation inside `JSONCrashLogParser.__new__`, have a 
static/class method for validation. Then, the base class can decide which 
subclass by calling the validation method. This means the subclasses don't need 
to override `__new__`. Ex:

```
class Base:
  def __new__(cls, i: int):
if Sub1.is_valid(i):
  return object.__new__(Sub1)
else:
  return object.__new__(Sub2)

  def __init__(self, i: int):
print("Base", i)

class Sub1(Base):
  @staticmethod
  def is_valid(i: int):
return i == 1234

  def __init__(self, i: int):
super().__init__(i)
print("Sub1", i)

class Sub2(Base):
  def __init__(self, i: int):
super().__init__(i)
print("Sub2", i)
```


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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Dave Lee via Phabricator via lldb-commits]

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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 2 inline comments as done.
mib added inline comments.



Comment at: lldb/examples/python/crashlog.py:434
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return  object().__new__(TextCrashLogParser)
 

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.


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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/examples/python/crashlog.py:434
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return  object().__new__(TextCrashLogParser)
 

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?


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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Dave Lee via Phabricator via lldb-commits]

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


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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 2 inline comments as done.
mib added inline comments.



Comment at: lldb/examples/python/crashlog.py:434
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return  object().__new__(TextCrashLogParser)
 

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.


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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere 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:
> 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)
```


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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 3 inline comments as done.
mib added inline comments.



Comment at: lldb/examples/python/crashlog.py:434
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return  object().__new__(TextCrashLogParser)
 

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.



Comment at: lldb/examples/python/crashlog.py:447
+class JSONCrashLogParser(CrashLogParser):
+def __new__(cls, debugger, path, verbose):
+with open(path, 'r') as f:

kastiglione wrote:
> can this be an `__init__`?
No unfortunately because at the `__init__` is called, the type of the object is 
already set, but we actually want to fallback to the `TextCrashLogParser` class 
if this one fails.

`__new__`: takes arguments and returns an instance
`__init__`: takes instance and arguments (from `__new__`) and returns nothing


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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/examples/python/crashlog.py:447
+class JSONCrashLogParser(CrashLogParser):
+def __new__(cls, debugger, path, verbose):
+with open(path, 'r') as f:

can this be an `__init__`?


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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/examples/python/crashlog.py:422-425
+# When `__new__` is overriden, if the returned type is the same
+# as the class type, or related to it, the `__init__` method is
+# automatically called after `__new__`. If the return type is not
+# related, then the `__init__` method is not called.

Is this comment needed (maybe it was in an earlier draft)? The body of the 
function isn't calling init, so it might be fine to not have it.



Comment at: lldb/examples/python/crashlog.py:434
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return  object().__new__(TextCrashLogParser)
 

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


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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 450694.
mib marked 2 inline comments as done.
mib added a comment.

Address @kastiglione comment


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

https://reviews.llvm.org/D131085

Files:
  lldb/examples/python/crashlog.py
  lldb/examples/python/scripted_process/crashlog_scripted_process.py

Index: lldb/examples/python/scripted_process/crashlog_scripted_process.py
===
--- lldb/examples/python/scripted_process/crashlog_scripted_process.py
+++ lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -10,10 +10,8 @@
 
 class CrashLogScriptedProcess(ScriptedProcess):
 def parse_crashlog(self):
-try:
-crash_log = CrashLogParser().parse(self.dbg, self.crashlog_path, False)
-except Exception as e:
-raise e
+crashlog_parser = CrashLogParser(self.dbg, self.crashlog_path, False)
+crash_log = crashlog_parser.parse()
 
 self.pid = crash_log.process_id
 self.addr_mask = crash_log.addr_mask
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -26,6 +26,7 @@
 #   PYTHONPATH=/path/to/LLDB.framework/Resources/Python ./crashlog.py ~/Library/Logs/DiagnosticReports/a.crash
 #--
 
+import abc
 import concurrent.futures
 import contextlib
 import datetime
@@ -415,19 +416,44 @@
 pass
 
 class CrashLogParser:
-def parse(self, debugger, path, verbose):
+"CrashLog parser base class and factory."
+
+def __new__(cls, debugger, path, verbose):
+# When `__new__` is overriden, if the returned type is the same
+# as the class type, or related to it, the `__init__` method is
+# automatically called after `__new__`. If the return type is not
+# related, then the `__init__` method is not called.
+
+# We don't need to propagate `__new__`'s aguments because both
+# `JSONCrashLogParser` & `TextCrashLogParser` are sub-classes of
+# `CrashLogParser`, so they will automagically be passed to the
+#  sub-class `__init__` method.
 try:
-return JSONCrashLogParser(debugger, path, verbose).parse()
+return JSONCrashLogParser.__new__(cls, debugger, path, verbose)
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return  object().__new__(TextCrashLogParser)
 
-
-class JSONCrashLogParser:
 def __init__(self, debugger, path, verbose):
 self.path = os.path.expanduser(path)
 self.verbose = verbose
 self.crashlog = CrashLog(debugger, self.path, self.verbose)
 
+@abc.abstractmethod
+def parse(self):
+pass
+
+
+class JSONCrashLogParser(CrashLogParser):
+def __new__(cls, debugger, path, verbose):
+with open(path, 'r') as f:
+buffer = f.read()
+try:
+self =  object().__new__(JSONCrashLogParser)
+self.data = self.parse_json(buffer)
+return self
+except:
+raise CrashLogFormatException()
+
 def parse_json(self, buffer):
 try:
 return json.loads(buffer)
@@ -438,14 +464,6 @@
 return json.loads(tail)
 
 def parse(self):
-with open(self.path, 'r') as f:
-buffer = f.read()
-
-try:
-self.data = self.parse_json(buffer)
-except:
-raise CrashLogFormatException()
-
 try:
 self.parse_process_info(self.data)
 self.parse_images(self.data['usedImages'])
@@ -592,7 +610,7 @@
 INSTRS = 5
 
 
-class TextCrashLogParser:
+class TextCrashLogParser(CrashLogParser):
 parent_process_regex = re.compile('^Parent Process:\s*(.*)\[(\d+)\]')
 thread_state_regex = re.compile('^Thread ([0-9]+) crashed with')
 thread_instrs_regex = re.compile('^Thread ([0-9]+) instruction stream')
@@ -615,13 +633,10 @@
   r'(/.*)'   # img_path
  )
 
-
 def __init__(self, debugger, path, verbose):
-self.path = os.path.expanduser(path)
-self.verbose = verbose
+super().__init__(debugger, path, verbose)
 self.thread = None
 self.app_specific_backtrace = False
-self.crashlog = CrashLog(debugger, self.path, self.verbose)
 self.parse_mode = CrashLogParseMode.NORMAL
 self.parsers = {
 CrashLogParseMode.NORMAL : self.parse_normal,
@@ -1012,7 +1027,7 @@
 if not os.path.exists(crashlog_path):
 raise InteractiveCrashLogException("crashlog file %s does not exist" % crashlog_path)
 
-crashlog = CrashLogParser().parse(debugger, crashlog_path, False)
+crashlog = CrashLogParser(debugger, 

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/examples/python/crashlog.py:417
+class CrashLogParser(object):
+"Base class and factory."
+def __new__(cls, debugger, path, verbose, *args, **kwargs):

JDevlieghere wrote:
> 
This is not done (yet)


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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 449847.
mib added a comment.

Address @JDevlieghere comments


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

https://reviews.llvm.org/D131085

Files:
  lldb/examples/python/crashlog.py
  lldb/examples/python/scripted_process/crashlog_scripted_process.py

Index: lldb/examples/python/scripted_process/crashlog_scripted_process.py
===
--- lldb/examples/python/scripted_process/crashlog_scripted_process.py
+++ lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -10,10 +10,8 @@
 
 class CrashLogScriptedProcess(ScriptedProcess):
 def parse_crashlog(self):
-try:
-crash_log = CrashLogParser().parse(self.dbg, self.crashlog_path, False)
-except Exception as e:
-raise e
+crashlog_parser = CrashLogParser(self.dbg, self.crashlog_path, False)
+crash_log = crashlog_parser.parse()
 
 self.pid = crash_log.process_id
 self.addr_mask = crash_log.addr_mask
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -26,6 +26,7 @@
 #   PYTHONPATH=/path/to/LLDB.framework/Resources/Python ./crashlog.py ~/Library/Logs/DiagnosticReports/a.crash
 #--
 
+import abc
 import concurrent.futures
 import contextlib
 import datetime
@@ -414,20 +415,28 @@
 class InteractiveCrashLogException(Exception):
 pass
 
-class CrashLogParser:
-def parse(self, debugger, path, verbose):
+class CrashLogParser(object):
+"CrashLog parser base class and factory."
+def __new__(cls, debugger, path, verbose, *args, **kwargs):
 try:
-return JSONCrashLogParser(debugger, path, verbose).parse()
+return super(CrashLogParser, cls).__new__(JSONCrashLogParser, *args, **kwargs)
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return super(CrashLogParser, cls).__new__(TextCrashLogParser, *args, **kwargs)
 
-
-class JSONCrashLogParser:
 def __init__(self, debugger, path, verbose):
 self.path = os.path.expanduser(path)
 self.verbose = verbose
 self.crashlog = CrashLog(debugger, self.path, self.verbose)
 
+@abc.abstractmethod
+def parse(self):
+pass
+
+
+class JSONCrashLogParser(CrashLogParser):
+def __init__(self, debugger, path, verbose):
+super().__init__(debugger, path, verbose)
+
 def parse_json(self, buffer):
 try:
 return json.loads(buffer)
@@ -592,7 +601,7 @@
 INSTRS = 5
 
 
-class TextCrashLogParser:
+class TextCrashLogParser(CrashLogParser):
 parent_process_regex = re.compile('^Parent Process:\s*(.*)\[(\d+)\]')
 thread_state_regex = re.compile('^Thread ([0-9]+) crashed with')
 thread_instrs_regex = re.compile('^Thread ([0-9]+) instruction stream')
@@ -617,11 +626,9 @@
 
 
 def __init__(self, debugger, path, verbose):
-self.path = os.path.expanduser(path)
-self.verbose = verbose
+super().__init__(debugger, path, verbose)
 self.thread = None
 self.app_specific_backtrace = False
-self.crashlog = CrashLog(debugger, self.path, self.verbose)
 self.parse_mode = CrashLogParseMode.NORMAL
 self.parsers = {
 CrashLogParseMode.NORMAL : self.parse_normal,
@@ -1012,7 +1019,7 @@
 if not os.path.exists(crashlog_path):
 raise InteractiveCrashLogException("crashlog file %s does not exist" % crashlog_path)
 
-crashlog = CrashLogParser().parse(debugger, crashlog_path, False)
+crashlog = CrashLogParser(debugger, crashlog_path, False).parse()
 
 target = lldb.SBTarget()
 # 1. Try to use the user-provided target
@@ -1257,7 +1264,7 @@
 except InteractiveCrashLogException as e:
 result.SetError(str(e))
 else:
-crash_log = CrashLogParser().parse(debugger, crash_log_file, options.verbose)
+crash_log = CrashLogParser(debugger, crash_log_file, options.verbose).parse()
 SymbolicateCrashLog(crash_log, options)
 
 if __name__ == '__main__':
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 4 inline comments as done.
mib added inline comments.



Comment at: lldb/examples/python/crashlog.py:428
 self.crashlog = CrashLog(debugger, self.path, self.verbose)
+self.data = None
+

mib wrote:
> JDevlieghere wrote:
> > Can this stay in the JSONCrashLogParser?
> I need it to get the `exception` dictionary for D131086 ... 
I took another look at this ... it seems that `TextCrashLogParser` doesn't set 
the `data` or `exception` attributes ... I'd need first to have both parsers 
produce the same `exception` dictionary before being able to use it. I'll do 
that on a follow-up patch.


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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/examples/python/crashlog.py:428
 self.crashlog = CrashLog(debugger, self.path, self.verbose)
+self.data = None
+

JDevlieghere wrote:
> Can this stay in the JSONCrashLogParser?
I need it to get the `exception` dictionary for D131086 ... 


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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/examples/python/crashlog.py:417
+class CrashLogParser(object):
+"Base class and factory."
+def __new__(cls, debugger, path, verbose, *args, **kwargs):





Comment at: lldb/examples/python/crashlog.py:428
 self.crashlog = CrashLog(debugger, self.path, self.verbose)
+self.data = None
+

Can this stay in the JSONCrashLogParser?



Comment at: lldb/examples/python/crashlog.py:630
 def __init__(self, debugger, path, verbose):
-self.path = os.path.expanduser(path)
-self.verbose = verbose
+super().__init__(debugger,path,verbose)
 self.thread = None





Comment at: 
lldb/examples/python/scripted_process/crashlog_scripted_process.py:15
+crashlog_parser = CrashLogParser(self.dbg, self.crashlog_path, 
False)
+crashlog = crashlog_parser.parse()
 except Exception as e:

Given that the module itself is called `crashlog`, I would keep the underscore 
to avoid potential naming collisions. 


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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/examples/python/crashlog.py:418-422
+def __new__(cls, debugger, path, verbose, *args, **kwargs):
 try:
-return JSONCrashLogParser(debugger, path, verbose).parse()
+return super(CrashLogParser, cls).__new__(JSONCrashLogParser, 
*args, **kwargs)
 except CrashLogFormatException:
+return super(CrashLogParser, cls).__new__(TextCrashLogParser, 
*args, **kwargs)

It looks like `debugger, path, verbose` are not propagated to the 
`CrashLogParser` subclass, is this an oversight or is there some magic? 
Secondly, `args` and `kwargs` used/needed?



Comment at: 
lldb/examples/python/scripted_process/crashlog_scripted_process.py:16-17
+crashlog = crashlog_parser.parse()
 except Exception as e:
 raise e
 

Is this catch and re-raise of any use? Since you're refactoring, maybe delete 
it?


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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 449725.
mib added a comment.

Add missing `import`


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

https://reviews.llvm.org/D131085

Files:
  lldb/examples/python/crashlog.py
  lldb/examples/python/scripted_process/crashlog_scripted_process.py

Index: lldb/examples/python/scripted_process/crashlog_scripted_process.py
===
--- lldb/examples/python/scripted_process/crashlog_scripted_process.py
+++ lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -11,13 +11,14 @@
 class CrashLogScriptedProcess(ScriptedProcess):
 def parse_crashlog(self):
 try:
-crash_log = CrashLogParser().parse(self.dbg, self.crashlog_path, False)
+crashlog_parser = CrashLogParser(self.dbg, self.crashlog_path, False)
+crashlog = crashlog_parser.parse()
 except Exception as e:
 raise e
 
-self.pid = crash_log.process_id
-self.addr_mask = crash_log.addr_mask
-self.crashed_thread_idx = crash_log.crashed_thread_idx
+self.pid = crashlog.process_id
+self.addr_mask = crashlog.addr_mask
+self.crashed_thread_idx = crashlog.crashed_thread_idx
 self.loaded_images = []
 
 def load_images(self, images):
@@ -35,12 +36,12 @@
 else:
 self.loaded_images.append(image)
 
-for thread in crash_log.threads:
+for thread in crashlog.threads:
 if self.load_all_images:
-load_images(self, crash_log.images)
+load_images(self, crashlog.images)
 elif thread.did_crash():
 for ident in thread.idents:
-load_images(self, crash_log.find_images_with_identifier(ident))
+load_images(self, crashlog.find_images_with_identifier(ident))
 
 self.threads[thread.index] = CrashLogScriptedThread(self, None, thread)
 
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -26,6 +26,7 @@
 #   PYTHONPATH=/path/to/LLDB.framework/Resources/Python ./crashlog.py ~/Library/Logs/DiagnosticReports/a.crash
 #--
 
+import abc
 import concurrent.futures
 import contextlib
 import datetime
@@ -412,19 +413,28 @@
 pass
 
 
-class CrashLogParser:
-def parse(self, debugger, path, verbose):
+class CrashLogParser(object):
+"Base class and factory."
+def __new__(cls, debugger, path, verbose, *args, **kwargs):
 try:
-return JSONCrashLogParser(debugger, path, verbose).parse()
+return super(CrashLogParser, cls).__new__(JSONCrashLogParser, *args, **kwargs)
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return super(CrashLogParser, cls).__new__(TextCrashLogParser, *args, **kwargs)
 
-
-class JSONCrashLogParser:
 def __init__(self, debugger, path, verbose):
 self.path = os.path.expanduser(path)
 self.verbose = verbose
 self.crashlog = CrashLog(debugger, self.path, self.verbose)
+self.data = None
+
+@abc.abstractmethod
+def parse(self):
+pass
+
+
+class JSONCrashLogParser(CrashLogParser):
+def __init__(self, debugger, path, verbose):
+super().__init__(debugger, path, verbose)
 
 def parse_json(self, buffer):
 try:
@@ -592,7 +602,7 @@
 INSTRS = 5
 
 
-class TextCrashLogParser:
+class TextCrashLogParser(CrashLogParser):
 parent_process_regex = re.compile('^Parent Process:\s*(.*)\[(\d+)\]')
 thread_state_regex = re.compile('^Thread ([0-9]+) crashed with')
 thread_instrs_regex = re.compile('^Thread ([0-9]+) instruction stream')
@@ -617,11 +627,9 @@
 
 
 def __init__(self, debugger, path, verbose):
-self.path = os.path.expanduser(path)
-self.verbose = verbose
+super().__init__(debugger,path,verbose)
 self.thread = None
 self.app_specific_backtrace = False
-self.crashlog = CrashLog(debugger, self.path, self.verbose)
 self.parse_mode = CrashLogParseMode.NORMAL
 self.parsers = {
 CrashLogParseMode.NORMAL : self.parse_normal,
@@ -1014,7 +1022,7 @@
 if not os.path.exists(crashlog_path):
 raise Exception("crashlog file %s does not exist" % crashlog_path)
 
-crashlog = CrashLogParser().parse(debugger, crashlog_path, False)
+crashlog = CrashLogParser(debugger, crashlog_path, False).parse()
 
 target = lldb.SBTarget()
 # 1. Try to use the user-provided target
@@ -1261,7 +1269,7 @@
 err.SetErrorString(str(e))
 result.SetError(err)
 else:
-crash_log = CrashLogParser().parse(debugger, 

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib created this revision.
mib added reviewers: JDevlieghere, kastiglione.
mib added a project: LLDB.
Herald added a project: All.
mib requested review of this revision.
Herald added a subscriber: lldb-commits.

This patch changes the CrashLogParser class to be both the base class
and a Factory for the JSONCrashLogParser & TextCrashLogParser.

That should help remove some code duplication and ensure both class
have a parse method.

Signed-off-by: Med Ismail Bennani 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131085

Files:
  lldb/examples/python/crashlog.py
  lldb/examples/python/scripted_process/crashlog_scripted_process.py

Index: lldb/examples/python/scripted_process/crashlog_scripted_process.py
===
--- lldb/examples/python/scripted_process/crashlog_scripted_process.py
+++ lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -11,13 +11,14 @@
 class CrashLogScriptedProcess(ScriptedProcess):
 def parse_crashlog(self):
 try:
-crash_log = CrashLogParser().parse(self.dbg, self.crashlog_path, False)
+crashlog_parser = CrashLogParser(self.dbg, self.crashlog_path, False)
+crashlog = crashlog_parser.parse()
 except Exception as e:
 raise e
 
-self.pid = crash_log.process_id
-self.addr_mask = crash_log.addr_mask
-self.crashed_thread_idx = crash_log.crashed_thread_idx
+self.pid = crashlog.process_id
+self.addr_mask = crashlog.addr_mask
+self.crashed_thread_idx = crashlog.crashed_thread_idx
 self.loaded_images = []
 
 def load_images(self, images):
@@ -35,12 +36,12 @@
 else:
 self.loaded_images.append(image)
 
-for thread in crash_log.threads:
+for thread in crashlog.threads:
 if self.load_all_images:
-load_images(self, crash_log.images)
+load_images(self, crashlog.images)
 elif thread.did_crash():
 for ident in thread.idents:
-load_images(self, crash_log.find_images_with_identifier(ident))
+load_images(self, crashlog.find_images_with_identifier(ident))
 
 self.threads[thread.index] = CrashLogScriptedThread(self, None, thread)
 
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -412,19 +412,28 @@
 pass
 
 
-class CrashLogParser:
-def parse(self, debugger, path, verbose):
+class CrashLogParser(object):
+"Base class and factory."
+def __new__(cls, debugger, path, verbose, *args, **kwargs):
 try:
-return JSONCrashLogParser(debugger, path, verbose).parse()
+return super(CrashLogParser, cls).__new__(JSONCrashLogParser, *args, **kwargs)
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return super(CrashLogParser, cls).__new__(TextCrashLogParser, *args, **kwargs)
 
-
-class JSONCrashLogParser:
 def __init__(self, debugger, path, verbose):
 self.path = os.path.expanduser(path)
 self.verbose = verbose
 self.crashlog = CrashLog(debugger, self.path, self.verbose)
+self.data = None
+
+@abc.abstractmethod
+def parse(self):
+pass
+
+
+class JSONCrashLogParser(CrashLogParser):
+def __init__(self, debugger, path, verbose):
+super().__init__(debugger, path, verbose)
 
 def parse_json(self, buffer):
 try:
@@ -592,7 +601,7 @@
 INSTRS = 5
 
 
-class TextCrashLogParser:
+class TextCrashLogParser(CrashLogParser):
 parent_process_regex = re.compile('^Parent Process:\s*(.*)\[(\d+)\]')
 thread_state_regex = re.compile('^Thread ([0-9]+) crashed with')
 thread_instrs_regex = re.compile('^Thread ([0-9]+) instruction stream')
@@ -617,11 +626,9 @@
 
 
 def __init__(self, debugger, path, verbose):
-self.path = os.path.expanduser(path)
-self.verbose = verbose
+super().__init__(debugger,path,verbose)
 self.thread = None
 self.app_specific_backtrace = False
-self.crashlog = CrashLog(debugger, self.path, self.verbose)
 self.parse_mode = CrashLogParseMode.NORMAL
 self.parsers = {
 CrashLogParseMode.NORMAL : self.parse_normal,
@@ -1014,7 +1021,7 @@
 if not os.path.exists(crashlog_path):
 raise Exception("crashlog file %s does not exist" % crashlog_path)
 
-crashlog = CrashLogParser().parse(debugger, crashlog_path, False)
+crashlog = CrashLogParser(debugger, crashlog_path, False).parse()
 
 target = lldb.SBTarget()
 # 1. Try to use the user-provided target
@@ -1261,7 +1268,7 @@
 err.SetErrorString(str(e))
 result.SetError(err)