labath added inline comments.
================
Comment at: lldb/source/Commands/CommandObjectMemory.cpp:1710
result.SetStatus(eReturnStatusFailed);
}
}
----------------
DavidSpickett wrote:
> labath wrote:
> > How about adding `return false` here to avoid indenting the code below?
> I was trying to match the rest, but yes there is no sense making it more
> complicated.
Yeah, that's the old style of things, but we're currently changing things to
use early exits where possible. If I was doing this, I'd early-exitify the
whole function, but I did not want to make you do that for such a simple fix.
================
Comment at:
lldb/test/API/functionalities/memory-region/TestMemoryRegion.py:44-52
+ # Test that when the address fails to parse, we do not carry on
+ # and ask lldb-server for an invalid address
+ interp.HandleCommand("memory region not_an_address", result)
+ self.assertFalse(result.Succeeded())
+ self.assertRegexpMatches(result.GetError(),
+ "error: invalid address argument \"not_an_address\"")
+ # This would be found if we carried on despite the error
----------------
DavidSpickett wrote:
> labath wrote:
> > If you're goal is to ensure that lldb-server is really not queried, then
> > this would require a different kind of a test.
> >
> > But if (as I suspect), you just want to ensure the error message makes
> > sense, then it would be better to just match the full text of the error
> > message, and drop all references to lldb-server.
> It's both, that you get an error that makes sense but also that we don't then
> carry on regardless. I suppose there's no guarantee that that second error is
> caused by sending a qMemoryRegion packet so it isn't very exact.
>
> I'll add a second test that checks that the specific packets are not sent
> after the first error.
>
Right, but this won't check that the second message is not displayed, will it?
I thought we'd change this to an exact string match instead of a regex. It
won't be consistent with the surrounding code, but it could be argued that the
surrounding code should also be changed to use exact matches in order to ensure
the error messages are really reasonable...
I'm not convinced of the value of the other test, but I won't stop you from
adding it. :)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87694/new/
https://reviews.llvm.org/D87694
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
