bulbazord requested changes to this revision.
bulbazord added a comment.
This revision now requires changes to proceed.
Idea is good, few concerns about the implementation.
================
Comment at: lldb/source/Target/Process.cpp:404-419
+llvm::StringRef Process::GetAttachSynchronousHijackListenerName() {
+ static ConstString class_name(
+ "lldb.internal.Process.AttachSynchronous.hijack");
+ return class_name.GetCString();
+}
+
+llvm::StringRef Process::GetLaunchSynchronousHijackListenerName() {
----------------
Do these need to be ConstStrings? They will live in the string pool forever,
and it looks like in the code below you're just manipulating `const char *`
anyway.
Could be something like:
```
llvm::StringRef Process::GetAttachSynchronousHijackListenerName() {
return "lldb.internal.Process.AttachSynchronous.hijack";
}
```
================
Comment at: lldb/source/Target/Process.cpp:1427
if (hijacking_name &&
- strcmp(hijacking_name, g_resume_sync_name))
+ strstr(hijacking_name, "lldb.internal") != hijacking_name)
return true;
----------------
Instead of using the `strstr` function directly, I would at least do `strnstr`
to ensure that we're not touching memory we don't have. Especially if we could
get `hijacking_name` to be shorter than `"lldb.internal"` somehow...
We could also change this to use StringRefs and use the
`starts_with`/`startswith` methods instead.
================
Comment at: lldb/source/Target/Process.cpp:1437-1438
if (hijacking_name &&
- strcmp(hijacking_name, g_resume_sync_name) == 0)
+ strcmp(hijacking_name,
+ GetResumeSynchronousHijackListenerName().data()) == 0)
return true;
----------------
First, I would probably avoid `strcmp` directly and use `strncmp` instead. We
know the length of the HijackListener name, that would be a good choice for `n`
here.
But you could probably simplify this even further. You can compare
`GetResumeSynchronousHijackListenerName` to `hijacking_name` with StringRef's
operator==.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148400/new/
https://reviews.llvm.org/D148400
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
