xiaobai added inline comments.
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/comp_dir_symlink/TestCompDirSymLink.py:39-40
(_COMP_DIR_SYM_LINK_PROP, pwd_symlink))
- lldbutil.run_break_set_by_file_and_line(self, self.src_path, self.line)
+ src_path = self.getBuildArtifact(_SRC_FILE)
+ lldbutil.run_break_set_by_file_and_line(self, src_path, self.line)
----------------
Instead of making a temporary variable `src_path` that gets used once in each
instance, why not just insert `self.getBuildArtifact(_SRC_FILE)` directly into
`lldbutil.run_break_set_by_file_and_line()`?
================
Comment at:
packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_on_vectors/TestValueOfVectorVariable.py:25-26
"""Test verify displayed value of vector variable."""
- self.build(dictionary=self.d)
- self.setTearDownCleanup(dictionary=self.d)
+ exe = self.getBuildArtifact("a.out")
+ d = {'C_SOURCES': self.source, 'EXE': exe}
+ self.build(dictionary=d)
----------------
You should be able to insert the call to `getBuildArtifact()` into the
construction of `d`, since `exe` is used nowhere else.
================
Comment at:
packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_on_vectors/TestValueOfVectorVariable.py:40
exe = self.getBuildArtifact("a.out")
+ d = {'C_SOURCES': self.source, 'EXE': exe}
self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
----------------
Is this `d` used?
================
Comment at: packages/Python/lldbsuite/test/lldbinline.py:96-98
+ return "-N dwarf %s" % (testdir)
else:
+ return "-N dsym %s" % (testdir)
----------------
Good opportunity to move away from the old style of formatting strings to a
newer style? It doesn't make a huge difference, I just think it'd be nice to
do. :)
`return "-N dwarf {}".format(testdir)`
`return "-N dwarf {}".format(testdir)`
This is supported in Python 2.7, but I'm not sure if we can assume that version
or greater to be present.
================
Comment at: packages/Python/lldbsuite/test/lldbtest.py:533-539
+ if rel_prefix[-3:] == ".py":
+ rel_prefix = rel_prefix[:-3]
+ elif rel_prefix[-4:] == ".pyc":
+ rel_prefix = rel_prefix[:-4]
+ else:
+ raise Exception("test_file is not a python file")
+ return os.path.split(rel_prefix)
----------------
Might I suggest this?
```
(rel_test_path, extension) = os.path.splitext(rel_prefix)
if extension not in [".py", ".pyc"]:
raise Exception("test_file is not a python file")
return rel_test_path
```
Feels cleaner to me, at least. :)
================
Comment at: packages/Python/lldbsuite/test/lldbtest.py:712
"""Return the full path to the current test."""
- return os.path.join(os.environ["LLDB_TEST"], self.mydir)
+ subdir, stem = self.mydir
+ return os.path.join(os.environ["LLDB_TEST"], subdir)
----------------
`stem` is unused, you could make this a `_`.
================
Comment at: packages/Python/lldbsuite/test/lldbtest.py:718
+ variant = self.debug_info
+ subdir, stem = self.mydir
+ if not variant:
----------------
`subdir` and `stem` are kind of confusing to me. It looks like you used
`test_subdir` below, so it might be good to use that here too, I think. Also, I
think the name `test_name` is less confusing than `test_stem`. What do you
think?
================
Comment at: packages/Python/lldbsuite/test/lldbtest.py:719
+ subdir, stem = self.mydir
+ if not variant:
+ variant = 'default'
----------------
I recommend `if variant is None` instead of `if not variant`
================
Comment at: packages/Python/lldbsuite/test/plugins/builder_base.py:63
# Construct the base make invocation.
+ test_subdir, test_stem = dir_stem
lldb_test = os.environ["LLDB_TEST"]
----------------
I find `test_stem` kind of confusing. How do you feel about `test_name`?
https://reviews.llvm.org/D42763
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
