zturner added a comment.
This probably seems like a lot of comments, but most of them are pretty minor.
Overall this looks like a good patch. When running the test suite with this
patch, did any tests start showing up as Unexpected Success?
================
Comment at: packages/Python/lldbsuite/test/dotest.py:288-294
@@ -288,1 +287,9 @@
+
+ if sys.platform.startswith('win32'):
+ import ctypes, time
+ while not ctypes.windll.kernel32.IsDebuggerPresent():
+ time.sleep(0.5)
+ ctypes.windll.kernel32.DebugBreak()
+ else:
+ os.kill(os.getpid(), signal.SIGSTOP)
----------------
See if you can get [[ https://github.com/Microsoft/PTVS/releases | PTVS ]] to
work. If so, you won't need any of this code and it will be a much better
debugging experience anyway. Assuming PTVS works for you, I'd rather just
delete this code. I'm planning to add info about using PTVS to the website
this week.
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/TestBreakpointCaseSensitivity.py:20
@@ +19,3 @@
+
+ def test_case_sensitivity_breakpoint(self):
+ """Set breakpoint on file, should match files with different case on
Windows"""
----------------
When we run the test suite, it's going to report this test as
`test_case_sensitivity_breakpoint`. So looking at it, you will have to think
for a minute about whether the test was expecting it to match or to not match.
It would be nice if the test was called something like
`test_breakpoint_matches_file_with_different_case`. So then you would need 2
test methods. One for matching with different case, and one for not matching
with different case. Then you could use a decorator to enable them on windows
or on not windows. Like this:
```
@skipIf(oslist=no_match(['windows'])) # Skip for non-windows platforms
def test_breakpoint_matches_file_with_different_case(self):
self.build()
self.case_sensitivity_breakpoint(true)
@skipIf(oslist=['windows']) # Skip for windows platforms
def test_breakpoint_doesnt_match_file_with_different_case(self):
self.build()
self.case_sensitivity_breakpoint(false)
```
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/TestBreakpointCaseSensitivity.py:25-35
@@ +24,13 @@
+
+ def case_sensitivity_breakpoint(self):
+ """Set breakpoint on file, should match files with different case on
Windows"""
+
+ # if breakpoint should hit when file path is case modified
+ should_hit_modified_case = lldbplatformutil.getHostPlatform() ==
"windows"
+
+ # use different case on Windows
+ if should_hit_modified_case:
+ exe = os.path.join(os.getcwd(), "A.OUT")
+ else:
+ exe = os.path.join(os.getcwd(), "a.out")
+
----------------
Then this whole thing becomes:
```
def case_sensitivity_breakpoint(self, case_insensitive):
exe = "a.out"
if case_insensitive:
exe = strupper(exe)
exe = os.path.join(os.getcwd(), exe)
```
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/TestBreakpointCaseSensitivity.py:92-95
@@ +91,6 @@
+
+ # not needed to spawn the process when break is not set
+ if not should_hit:
+ self.target.BreakpointDelete(breakpoint.GetID())
+ return
+
----------------
If `should_hit` is false, then I think we should launch the process anyway, but
verify that it DOES NOT get hit. It would look something like this (not
tested, maybe some errors here):
```
process = self.target.LaunchSimple(None, None,
self.get_process_working_directory())
self.assertTrue(process, PROCESS_IS_VALID + desc)
if should_hit:
# Did we hit our breakpoint?
from lldbsuite.test.lldbutil import get_threads_stopped_at_breakpoint
threads = get_threads_stopped_at_breakpoint (process, breakpoint)
self.assertEqual(1, len(threads), "There should be a thread stopped at
breakpoint" + desc)
# The hit count for the breakpoint should be 1.
self.assertEqual(1, breakpoint.GetHitCount())
else:
self.assertEqual(lldb.eStateExited, process.GetState())
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/TestBreakpointCaseSensitivity.py:103
@@ +102,3 @@
+ threads = get_threads_stopped_at_breakpoint (process, breakpoint)
+ self.assertTrue(len(threads) == 1, "There should be a thread stopped
at breakpoint" + desc)
+ # The hit count for the breakpoint should be 1.
----------------
Minor nitpick, but can you use `assertEqual` here? Same goes for following
test. This will provide a better error message, since as the code is written
here it will just say "Expected True, got False", but with `assertEqual`, it
will say "expected 1, got 0" or something like that.
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/TestBreakpointCaseSensitivity.py:108
@@ +107,3 @@
+ # clean after ourself
+ process.Kill()
+ self.target.BreakpointDelete(breakpoint.GetID())
----------------
`process.Kill()` is kind of harsh. I know some tests do it, but usually we
just do `process.Continue()` to let it exit gracefully. (It shouldn't matter
in practice, so whatever you prefer)
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/TestBreakpointCaseSensitivity.py:109
@@ +108,2 @@
+ process.Kill()
+ self.target.BreakpointDelete(breakpoint.GetID())
----------------
This line is harmless, but not necessary. Because the test runner should end
up deleting the target later anyway.
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/TestBreakpointCaseSensitivity.py:110
@@ +109,1 @@
+ self.target.BreakpointDelete(breakpoint.GetID())
\ No newline at end of file
----------------
Need newline at end of file.
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/main.c:6
@@ +5,3 @@
+{
+ printf ("Set a breakpoint here.\n");
+ return 0;
----------------
Doesn't look like this is clang-formatted. We don't use a space between
function name and open parentheses. You'll see a lot of code that does this,
but after that code was written we agreed to remove the parentheses.
clang-format should remove it for you automatically.
================
Comment at: source/Core/ConstString.cpp:269
@@ +268,3 @@
+bool
+ConstString::Equals (const ConstString& lhs, const ConstString& rhs, const
bool case_sensitive)
+{
----------------
Looks like this code also isn't clang-formatted. If you build clang-format
(with `ninja clang-format`) and add its folder to your `PATH` environment
variable, then when your changes are staged in git, just run `git clang-format`.
If you've already committed you can uncommit with `git reset --soft HEAD`.
Then they will be staged again, then you can run `git clang-format`, then
re-add all the modified files, and then recommit.
================
Comment at: source/Core/ConstString.cpp:274-275
@@ +273,4 @@
+
+ if (case_sensitive)
+ return false;
+
----------------
I thought this was wrong at first, but then I realized it's right. Can you add
a comment above this line:
```
// Since the pointers weren't equal, and identical ConstStrings always have
identical pointers, the result must be false.
```
================
Comment at: source/Host/common/FileSpec.cpp:400
@@ +399,3 @@
+ // case sensitivity of file check
+ const bool case_sensitive = m_syntax != ePathSyntaxWindows;
+
----------------
There is a minor edge case that isn't handled here, when `*this` and `rhs` have
different syntaxes. Granted if that's ever the case, it's very unlikely that
the `FileSpecs` would be otherwise the same, but it's possible (especially if
`m_directory` is not set).
So I think we should add a method to `FileSpec`
```
bool IsCaseSensitive() const { return m_syntax != ePathSyntaxWindows; }
```
Then this line becomes:
```
const bool case_sensitive = IsCaseSensitive() || rhs.IsCaseSensitive();
```
Here the comparison should be case sensitive if *either* `FileSpec` requires a
case sensitive comparison, not just `*this`.
================
Comment at: source/Host/common/FileSpec.cpp:402
@@ -400,1 +401,3 @@
+
+ if (ConstString::Equals(m_filename, rhs.m_filename, case_sensitive))
{
----------------
Can you reverse this conditional and just early out if they don't match (I know
that wasn't your code, but seems to fit more with LLVM style.
================
Comment at: source/Host/common/FileSpec.cpp:522
@@ -518,3 +521,3 @@
{
- result = ConstString::Compare(a.m_directory, b.m_directory);
+ result = ConstString::Compare(a.m_directory, b.m_directory, a.m_syntax
!= ePathSyntaxWindows);
if (result)
----------------
If you add the `IsCaseSensitive()` method, then this would change here too (and
also you should check `a` and `b` just like I mentioned in the last comment.
Same goes for the rest of the places.
http://reviews.llvm.org/D17492
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
