JDevlieghere added inline comments.
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:52-53
+ return_struct.impl_addr =
+ class_getMethodImplementation_stret (return_struct.class_addr,
+ return_struct.sel_addr);
+ } else {
----------------
Nit: Seems like the formatting is a bit off. I would copy/paste this code in a
separate file, run clang-format over it, and paste it back.
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:637-643
// Also we will use the version of the lookup code that doesn't rely on the
// stret version of the function.
- m_lookup_implementation_function_code =
- g_lookup_implementation_no_stret_function_code;
+ m_lookup_implementation_function_code.append(
+ g_lookup_implementation_no_stret_function_code);
} else {
- m_lookup_implementation_function_code =
- g_lookup_implementation_with_stret_function_code;
+ m_lookup_implementation_function_code.append(
+ g_lookup_implementation_with_stret_function_code);
----------------
I don't understand why we need to have two versions of the code. IIUC, if
there's a "stret return lookup function", then if we pass `is_stret == true`
we'll use it. Otherwise we'll unconditionally use the straight lookup. Why do
we need two variants of the code at all? Can't we pass false to `is_stret` and
achieve the same result with the
`g_lookup_implementation_with_stret_function_code` variant?
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:816-818
+ if (curr_sym) {
+ sym_name = curr_sym->GetName().GetStringRef();
+ }
----------------
Nit: for consistency with the line below
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:1012-1015
+ if (log) {
+ LLDB_LOG(log, "Resolving call for class - {0} and selector - {1}",
+ isa_addr, sel_addr);
+ }
----------------
The `if (log)` check is redundant. It's the purpose of the macro to expand to
that that. Same below.
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:1074
+ if (sel_str_addr == LLDB_INVALID_ADDRESS || error.Fail()) {
+ if (log)
+ LLDB_LOG(log,
----------------
redundant
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.h:55-65
+ // These hold the code for the function that finds the implementation of
+ // an ObjC message send given the class & selector and the kind of dispatch.
+ // There are two variants depending on whether the platform uses a separate
+ // _stret passing convention (e.g. Intel) or not (e.g. ARM). The difference
+ // is only at the very end of the function, so the code is broken into the
+ // common prefix and the suffix, which get composed appropriately before
+ // the function gets compiled.
----------------
Nit: I think these should be doxygen comments, so ///. You can group them like
this:
================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.cpp:82
+ Log *log = GetLog(LLDBLog::Step);
+ if (log) {
+ LLDB_LOG(log, "Caching: class {0} selector {1} implementation {2}.",
----------------
redundant
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.h:404-407
+ if (class_addr == rhs.class_addr && sel_name == rhs.sel_name)
+ return true;
+ else
+ return false;
----------------
```
return class_addr == rhs.class_addr && sel_name == rhs.sel_name;
```
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.h:411-417
+ if (class_addr < rhs.class_addr)
+ return true;
+ else if (class_addr > rhs.class_addr)
+ return false;
+ else {
+ return ConstString::Compare(sel_name, rhs.sel_name);
+ }
----------------
```
if (class_addr < rhs.class_addr)
return true;
if (class_addr > rhs.class_addr)
return false;
return ConstString::Compare(sel_name, rhs.sel_name);
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123421/new/
https://reviews.llvm.org/D123421
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
