Yeah, I think we're in agreement that pass-by-value would be a better method here.
To be honest, I'm not sure if expecting a user to return a std::pair<bool, Val*> from a plugin function hook is more or less obvious than an explicit wrapper. One thing that makes this a little less obvious to me is that the usage of this construct won't be limited to bro folks reviewing this code: plugin authors will need to craft one of these (either ValWrapper or std::pair) and return them from the hook. std::pair might actually be a little easier, since that might make it more obvious what's happening there (since std::pair is vanilla C++) ... but it also might not.
How about I give std::pair try and we can move back to an explicit wrapper if it makes the code too difficult to work with? It should be a pretty small change either way.
Also, Jon: primary use of this construct (that I can think of) will be in HandlePluginResult and BroFunc::Call in Func.cc (since those deal with the HookFunctionCall interface) if you'd like to take a look. There's also https://github.com/cubic1271/bro-plugin-instrumentation to offer an example of a plugin that uses the updated interface - once I make this change in core, I'll update that plugin to reflect the new usage.
On a related note, C++11 would be cool for a lot of reasons (e.g. std::atomic), but that might be another ticket / a longer discussion
I think unless the API is designed to be generic, one reason std::pair<T1, T2> may be less obvious is that if it shows up in multiple places, one doesn’t really know at a glance that they express the same meaning/intention.
So one thing to keep in mind is that not many people will ever write
code using this; it's just for function hooks, and I imagine their use
will be limited. I'm personaly still leaning towards the pair, but
it's not a big deal, the main point was the pass-by-value, and I'm
fine either way. If we stick with the wrapper, we should however at
least rename to something more semantically meaninful than ValWrapper,
like ValOrNull (better ideas appreciated :).
Gilbert, please reset to merge request once you're done updating the
branch.
So one thing to keep in mind is that not many people will ever write
code using this; it's just for function hooks, and I imagine their use
will be limited. I'm personaly still leaning towards the pair, but
it's not a big deal, the main point was the pass-by-value, and I'm
fine either way.
Me too (I was just in a “let me help brainstorm pros/cons” mood that day).
If we stick with the wrapper, we should however at
least rename to something more semantically meaninful than ValWrapper,
like ValOrNull (better ideas appreciated :).
Not having to choose a name I guess is a “pro” for std::pair
