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 :) -Gilbert On 10/17/2014 11:53 AM, Jon Siwek (JIRA) wrote: > [ > https://bro-tracker.atlassian.net/browse/BIT-1270?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18410#comment-18410 > ] > > Jon Siwek commented on BIT-1270: > -------------------------------- > > Whether to pass by value and whether to use ValWrapper vs. std::pair seem > like separate issues/suggestions? > > I'd also make the suggestion to pass by value. Whether that value is > ValWrapper or std::pair I don't have a good feel for without reviewing the > code more. Maybe std::pair if it's used infrequently else ValWrapper just to > be able to chose the names of the fields (reading/writing code that uses > "x.first" and "x.second" can get confusing for me personally). > > (And std::tuple is C++11, right? Did we decide that's generally ok to use in > Bro now?) > >> topic/gilbert/plugin-api-tweak >> ------------------------------ >> >> Key: BIT-1270 >> URL: https://bro-tracker.atlassian.net/browse/BIT-1270 >> Project: Bro Issue Tracker >> Issue Type: Improvement >> Components: Bro >> Reporter: gclark >> Assignee: gclark >> >> This branch makes a few changes to the API: >> * Wraps values in a simple class (ValWrapper) that include an explicit >> processed / not processed flag (to avoid confusion with delayed / opaque >> invocations). >> * Adds a Frame argument to HookCallFunction >> * Adds support for Frame argument types to HookArgument >> * Adds support for ValWrapper argument types to HookArgument >> * Tweaks the plugin.hooks tests a bit to include new output (from additional >> argument) >> * Tweaks the plugin.api-version-mismatch to remove explicit home directory >> path via simple regex > > > -- > This message was sent by Atlassian JIRA > (v6.4-OD-07-004#64005) > _______________________________________________ > bro-dev mailing list > [email protected] > http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-dev _______________________________________________ bro-dev mailing list [email protected] http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-dev
