Nicely rewriting outparams

Automatic code rewriting business can be a little depressing sometimes. I tend to run into funny issues caused by CPP, oink limitations or just unpleasant-to-rewrite parts of C++. After banging my head against the wall due to all these issues I finally arrived at a workable approach for the easy part of the outparam rewrite.

Currently I have a dehydra script that finds all non-virtual getters that return either NS_OK or *NS_SOMETHING_IS_WRONG*. The script then outputs data for squash to base the rewrites on. Then squash takes over.

In order to preserve sanity, pretty-printing is not used at all for rewriting the getter functions. This way one doesn’t have to worry about oink generating invalid C++ and the output is much more aesthetically pleasing. Instead, squash finds interesting expressions in the .i file. Then it extracts the corresponding strings from .h/.cpp files. The strings are used to fudge the position information obtained from the .i file to vaguely correspond to the original source files. After various C++ string-foo, squash produces a promising looking patch like this.

This also relies on a fair amount of semantic information provided by elsa/oink. For example when removing a parameter, squash inserts a local variable with the same name and then removes all of the derefences of the old parameter. Since there could be multiple variables with the same name, squash relies on elsa’s variable resolution.

I think squash is now 50% feature complete with respect to outparam stuff. The other 50% is the hard part of rewriting all of the call-sites. I’m not counting easy parts like wrapping the return type in already_AddRefed<>, eliminating redundant assignments in the getter or removing the error variable once it is no longer needed.

4 comments

  1. Note that the very first example in the diff is incorrect – CreateHTMLImageFrame has three possible return conditions, a) no frame necessary b) frame created succesfully c) failed to create frame.

  2. Thanks. Looks like we can’t do an automatic rewrite if a function already uses null/not-null in the outparam for a special meaning. That’s why I made selecting candidates & rewriting separate stages. This way a human can check/trim the list of candidates and rerun the tool as many times as needed and when it is most convenient.

  3. Johnny Stenback

    One note, the patch in question generates changes that replace an out param with a local variable with the same name. There’s a naming convention that conflicts with this, which is that arguments are named ‘a’ followed by something, and something is camel cased (i.e. first character upper cased, etc).

    Seems reasonably straight forward to auto-rename to strip off the ‘a’ and lowercase the following character. Or would that be hard?

  4. Fixing the naming convention is pretty easy. Maintaining indentation is harder(once more code moves around), but doable.