Outparams: Take 2

Will Rid Code of Outparams!

I resumed my outparam rewriting work last week. Having fixed the CPP induced architectural limitation that I ran into, it was quite straight-forward to factor out squash’s rewriting code into a new tool. Unlike squash, outparamdel (creatively named new tool), can rewrite code precisely and reliably. I still don’t have end-of-ast-node information in every Elsa AST member, but I think I have added position info to enough AST nodes to be able to do most of the Mozilla 2 rewrites.

Last time I was working on this, I was a little unhappy with the amount of code that had to be changed in the callee. I also wasn’t sure how to handle the complexity of callsites where control flow depends on the error code returned by the callee. This time I chose the path of least resistance.

In the callee two variables replace the out-parameter. The first holds the return value and the second points at the first and is declared in the same way as the out-parameter.

@@ -2358,6 +2358,5 @@
-nsresult
-nsCSSFrameConstructor::CreateHTMLImageFrame(nsIContent* aContent,
- nsStyleContext* aStyleContext,
- ImageFrameCreatorFunc aFunc,
- nsIFrame** aFrame)
-{
+nsIFrame*
+nsCSSFrameConstructor::CreateHTMLImageFrame(nsIContent* aContent,
+ nsStyleContext* aStyleContext,
+ ImageFrameCreatorFunc aFunc)
+{
@@ -2364,1 +2364,3 @@
- *aFrame = nsnull;
+ nsIFrame* __aFrame = 0;
+ nsIFrame** aFrame = &__aFrame;
+ *aFrame = nsnull;
@@ -2371,1 +2371,1 @@
- return NS_ERROR_OUT_OF_MEMORY;
+ return nsnull;
@@ -2374,1 +2374,1 @@
- return NS_OK;
+ return __aFrame;

With a little more work one can get of one or even both local variables, but that will require more heuristics.

I also simplified call-site rewriting by wrapping the call to the callee function into a ternary expression when the error code is evaluated.

@@ -5290,2 +5290,1 @@
- rv = CreateHTMLImageFrame(aContent, aStyleContext, NS_NewImageFrame,
- &newFrame);
+ rv = ((newFrame = CreateHTMLImageFrame(aContent, aStyleContext, NS_NewImageFrame)) ? NS_OK : NS_ERROR_OUT_OF_MEMORY);

Obviously this could be improved upon by getting rid of rv altogether, but the trick works in all cases and can be improved-upon incrementally.

Problems & Limitations

  • Code within macros is detected, but not modified. Since macros are something that is easier to modify manually than with a tool, this isn’t a problem.
  • Indentation isn’t updated. I think this is solvable with a few more heuristics in the patcher.
  • Expression rewriting doesn’t work on Mac or Windows source-code. Mac is going to be relatively straight-forward to support. MCPP doesn’t understand all of the Mac strangeness yet and needs a loving and affectionate mac user to teach it the ways of Darwin. Windows is more work as it will require both updating the mingw Mozilla port and porting MCPP.

Coolness

nsCSSFrameConstructor.cpp:10383: nsCSSFrameConstructor::CreateContinuingTableFrame,6=newFrame,
nsCSSFrameConstructor.cpp:10993: nsCSSFrameConstructor::GetInsertionPoint,2=aParentFrame,
nsCSSFrameConstructor.cpp:2374: nsCSSFrameConstructor::CreateHTMLImageFrame,3=aFunc,
nsCSSFrameConstructor.cpp:4080: nsCSSFrameConstructor::ConstructDocElementTableFrame,2=childList,
nsCSSFrameConstructor.cpp:4477: nsCSSFrameConstructor::ConstructRootFrame,1=viewportFrame,
nsCSSFrameConstructor.cpp:4720: nsCSSFrameConstructor::ConstructRadioControlFrame,0=NS_NewGfxRadioControlFrame,
nsCSSFrameConstructor.cpp:4741: nsCSSFrameConstructor::ConstructCheckboxControlFrame,0=NS_NewGfxCheckboxControlFrame,

I wrote outparamdel and tested it on a convoluted manual testcase. Then I ran outparamdel mozilla-wide with the above input and the resulting code compiled! This was never the case prior to MCPP work since I found find new and exciting preprocessor madness to work-around and special case with every new rewrite.

Future

Going to make this a little more challenging by rewriting QueryInterface to return result by a return value which should affect the majority of functions in Mozilla.

5 comments

  1. Hey, this is great stuff! I’m glad you’re documenting your progress in this blog.

  2. What sort of strangeness does it not support? I’m assuming it doesn’t know about Objective-C at all, and most of the Mac code has ObjC and C++ intermingling. Is there anything beyond that?

  3. We’ll get objectiveC supported. It’s mostly todo with teaching MCPP to deal with OSX include paths.

  4. The below questions are posted here, because I inferred that this blog’s author is sharpening the bleeding edge of mozilla’s deCOMtamination. If I am wrong, the guidance where such issues belong is appreciated. A couple of questions:

    1. You plan to use return value instead of outparam to fetch QI result. Will the result still be addref’ed?

    2. Smart pointers like nsCOMPtr are facilitating RAII design pattern. One assigns all acquired resources to such pointers, uses the resources as long as it makes sense, and forgets about the resources as they lose value. Smart pointers do the rest of the job – releasing.

    I assume that if QI is not addref’ing, then if I use a smart pointer once, the reference I hold becomes owning. If I pass it further, it will always be addref’ed thereafter.

    If the above paragraph is correct, maybe it makes sense to introduce a third method to deal with reference counting like ‘Unref()’ which will reduce refcount, but won’t cause destruction. And nsReturnPtr smart pointer which will call Unref() instead of Release(), thus bringing the return value resource to pristine ‘unsmarted’ state and allowing to pass it further as a raw pointer.