Add templated handler for manual hooks #150
Conversation
|
@psychonic what would you suggest to fix the tests? |
mixern6
force-pushed
the
templated_vhooks
branch
from
92abf91
to
564a667
Compare
|
At a glance, this looks ABI-safe and source compatible which is my main concern. I'll take a more in-depth look soon. |
|
Looks like there are some build failures in the tests that need to be resolved, too. |
mixern6
force-pushed
the
templated_vhooks
branch
from
564a667
to
a6acddf
Compare
|
@dvander can't figure out the right way for sorting out the compilation errors. It looks like I can't simply rename the variables or place them into separate name spaces and yet I have to keep the extern ones in the header. What would you suggest? Do you think it is important to have the I'd like to make the constructor constexpr, but this playing with the memory addresses in msProto_ makes me fill uncomfortable. Would be nice to have something like |
mixern6
force-pushed
the
templated_vhooks
branch
from
a6acddf
to
4d9a9b5
Compare
mixern6
force-pushed
the
templated_vhooks
branch
5 times, most recently
from
e15241b
to
80d9750
Compare
mixern6
force-pushed
the
templated_vhooks
branch
from
80d9750
to
1c92d10
Compare
|
@dvander all the errors are fixed. Could you have a look please? |
| , pAssignOperator(assignOperator) | ||
| { | ||
| } | ||
|
|
||
| void *pNormalCtor; |
There was a problem hiding this comment.
I'd assign nullptr to these fields rather than use a constructor.
| PassInfo() | ||
| : size(0) | ||
| , type(0) | ||
| , flags(0) |
There was a problem hiding this comment.
I'd assign zero to the fields in-line rather than use a constructor.
|
|
||
| int operator()(bool store, IHookManagerInfo *hi) const | ||
| { | ||
| if(staticFunc_ != nullptr) |
There was a problem hiding this comment.
nit: space between "if" and "(".
There was a problem hiding this comment.
It looks like staticFunc_ and memberFunc_ are mutually exclusive here.
Can we assert((staticFunc_ && !memberFunc_) || (!staticFunc_ && memberFunc_)), use else instead, and eliminate the default return?
| return *this; | ||
| } | ||
|
|
||
| operator bool() const |
There was a problem hiding this comment.
Should this be marked "explicit"?
| private: | ||
| static void DeleteDelegate(ISHDelegate* delegate) | ||
| { | ||
| if(delegate) |
| } | ||
|
|
||
| private: | ||
| std::shared_ptr<ISHDelegate> ptr_; |
There was a problem hiding this comment.
Does this need to be a shared_ptr? Can it be unique_ptr?
| struct IHookManagerInfo | ||
| { | ||
| virtual void SetInfo(int hookman_version, int vtbloffs, int vtblidx, | ||
| ProtoInfo *proto, void *hookfunc_vfnptr) = 0; | ||
|
|
||
| virtual void SetInfo(int hookman_version, int vtbloffs, int vtblidx, |
There was a problem hiding this comment.
Overloads of virtual methods need to have a new name to avoid breaking compatibility on MSVC (it does a weird thing where it inverts the order). SetInfo2 is fine if you're feeling uncreative.
| * @param post Set to true if you want a post handler | ||
| */ | ||
|
|
||
| virtual int AddHook(Plugin plug, AddHookMode mode, void *iface, int thisptr_offs, const HookManagerPubFuncHandler &myHookMan, |
There was a problem hiding this comment.
Same here, needs a new name unfortunately. "AddNewHook", "AddHook2", "NewHook", "CreateHook", "Hook" - off the top of my head, some mediocre ideas.
|
This is a huge improvement to the API, thanks for working on this! I'm up against some tight deadlines this week so I was only able to get halfway through the review. I'll try to do the rest as soon as I can. Mostly, I'm looking for compatibility issues that would break existing binaries, and for any maintenance/readability issues in the new code. There's not much: it's a great change, but it's also a big one and I want to do it justice. Please do include tests for the new API. |
Add templated wrapper for manual hooks. Simple hooks are not supported yet and will be added later.
The handler supports any return type and arbitrary number of arguments.
A hook can be defined like this:
// void - return type// const char* - first argumentSourceHook::ManualHookHandler<void, const char*> setEntityModelHook;Then it can be reconfigured:
setEntityModelHook.Reconfigure(offset);And this way it can be connected to a callback method:
const int hookID = setEntityModelHook.Add(baseEntityPtr, this, &MyClass::OnSetEntityModel, true);Remove the hook:
g_SHPtr->RemoveHookByID(hookID);the Recall method can be used instead of the RETURN_META* macros.