[JIRA] (BIT-1270) topic/gilbert/plugin-api-tweak

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 :slight_smile:

-Gilbert

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.

- Jon

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.

Robin

Maybe OptionalVal.

Robin

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 :slight_smile:

- Jon