labath added a comment.
I'm glad to see this getting fixed. I have a couple comments though. :)
================
Comment at: packages/Python/lldbsuite/test/dotest.py:695
@@ -694,3 +694,3 @@
# Try to match the regexp pattern, if specified.
if configuration.regexp:
----------------
This function (`visit`) is getting quite big, and due to the way it's
structured, you were forced to duplicate some code. We could solve both
problems with a bit of refactoring.
How about this:
- put everything inside the for loop below this line into a separate function
(`visit_file` ?)
- put the try-catch block around the call to `visit_file` in this function
This way we will have less duplication and will reduce the nesting depth of
this function.
What do you think?
================
Comment at: packages/Python/lldbsuite/test/dotest.py:725
@@ +724,3 @@
+ test_filename = os.path.abspath(os.path.join(dir, name))
+ if test_filename.endswith(".pyc"):
+ test_filename = test_filename[0:-1]
----------------
Will this ever be executed? I mean, if we get a pyc file, we shouldn't even get
here due to the check on line 689.
================
Comment at: packages/Python/lldbsuite/test_event/formatter/pickled.py:57
@@ +56,3 @@
+ # end.
+ if self.use_send:
+ # Send it as
{serialized_length_of_serialized_bytes}{serialized_bytes}
----------------
Why do we need the format to be different based on the kind of object we are
writing to? The added magic (introspection) seems like a bad tradeoff, if all
it does is avoid a couple of lines in `event_collector.py`
================
Comment at:
packages/Python/lldbsuite/test_event/test/src/TestCatchInvalidDecorator.py:38
@@ +37,3 @@
+
+ self.assertTrue(
+ len(results) > 0,
----------------
I don't fully understand the whole event collector logic yet, but I would
expect to see a check that the returned result is in fact an error result, no?
(Also, I prefer using `assertGreater(foo, bar)`, as it provides a more
descriptive error message.)
================
Comment at: packages/Python/lldbsuite/test_event/test/src/event_collector.py:25
@@ +24,3 @@
+def pickled_events_filename():
+ return "/tmp/lldb_test_event_pickled_event_output.dat"
+
----------------
I'm pretty sure this won't work on windows.. :)
`tempfile.NamedTemporaryFile` looks like a portable alternative.
http://reviews.llvm.org/D20193
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
