99421 - [FIX] if cascade cleanup

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Ulrich Drepper]

layout/html/style/src/nsCSSFrameConstructor.cpp contains a if cascade which can
ge cleaned up significantly with the result of fewer generated code (124 bytes
worth in my case).

Comment 1

[Ulrich Drepper]

Attachment: cleanup patch

Comment 2

[Marc Attinasi]

Personally, I like the part here:

+    if (val.EqualsIgnoreCase("submit") ||
+        val.EqualsIgnoreCase("reset") ||
+        val.EqualsIgnoreCase("button") ||
+        val.EqualsIgnoreCase("hidden")) {
+      return ConstructButtonControlFrame(aPresShell, aPresContext, aFrame);

but I don't like the scads of early returns. Why is returning at each case
better than setting the return value and then returning at the bottom of the
method? To skip one else statement?

Comment 3

[Ulrich Drepper]

> but I don't like the scads of early returns. Why is returning at each case
> better than setting the return value and then returning at the bottom of the
> method? To skip one else statement?

It allows to fold more cases.  Otherwise you'd have to operate with some local
variable to keep track of what has been done.  I don't like this.  Early return
statements are no problem.  The function has a very specific purpose and this is
to call one other function.  Writing the code this way also makes it easier (and
likelier) for the compiler to recognize tail calls and optimize appropriately. 
With a local variable this is not likely to happen.

Comment 4

[Marc Attinasi]

I think you only save one else statement with the early returns:

nsresult
nsCSSFrameConstructor::CreateInputFrame(nsIPresShell    *aPresShell,
                                        nsIPresContext  *aPresContext,
                                        nsIContent      *aContent, 
                                        nsIFrame        *&aFrame,
                                        nsIStyleContext *aStyleContext)
{
  nsresult  rv;

  // Figure out which type of input frame to create
  nsAutoString  val;
  if (NS_OK == aContent->GetAttr(kNameSpaceID_HTML, nsHTMLAtoms::type, val)) {
    if (val.EqualsIgnoreCase("submit") || 
	    val.EqualsIgnoreCase("reset")  ||
		val.EqualsIgnoreCase("button") ||
		val.EqualsIgnoreCase("hidden")) {
      rv = ConstructButtonControlFrame(aPresShell, aPresContext, aFrame);
    }
    else if (val.EqualsIgnoreCase("checkbox")) {
      rv = ConstructCheckboxControlFrame(aPresShell, aPresContext, aFrame, 
                                         aContent, aStyleContext);
    }
    else if (val.EqualsIgnoreCase("file")) {
      rv = NS_NewFileControlFrame(aPresShell, &aFrame);
    }
    else if (val.EqualsIgnoreCase("image")) {
      rv = NS_NewImageControlFrame(aPresShell, &aFrame);
    }
    else if (val.EqualsIgnoreCase("radio")) {
      rv = ConstructRadioControlFrame(aPresShell, aPresContext, aFrame, 
                                      aContent, aStyleContext);
    }
    else {
      // here is the extra else
      rv = ConstructTextControlFrame(aPresShell, aPresContext, aFrame, 
                                     aContent);
    }
    return rv;
  }
  rv = ConstructTextControlFrame(aPresShell, aPresContext, aFrame, aContent);
  
  return rv;
}

no local variables or anything like that needed.

... But I don't know what a tail call is, nor do I know how likely or important
it is for the compiler to optimize it. Anyway, there are not that many early
returns, and the method is still pretty manageable, so I'll take it your way,
with pleasure!  

[s]r=attinasi

Comment 5

[Ulrich Drepper]

Why for 1.0?  It already is reviewed?  Somebody just has to apply it.  Actually,
I'll update it first ("hidden" is handled differently today) but then somebody
just has to do it.  I don't have write access which is why it hasn't happened yet.
Maybe if I add footprint to the keywords...

Comment 6

[Ulrich Drepper]

Attachment: Updated patch

Updated patch which handles "hidden" buttons like the current code.

Comment 7

[Ulrich Drepper]

Come on, this patch is obviously correct and is reviewed and it helps to reduce
the footprint.  And nevertheless you push it out to 1.2?  This is a good way to
discourage anybody from contributing.  The only reason I haven't checked in the
patch is that I have no write access.  Somebody please simply check it in.

Comment 8

[Marc Attinasi]

Ulrich, Kevin made a mistake pusing this out, and I apologize also for having
forgotten about it.

Yes, we should check it in, but we still need a review for it (I have
super-reviewed it but we need review too). I'll ping Rods for the r= and then
check it in.
Thanks again, and sorry for the mistake.

Comment 9

[rods (gone)]

r=rods

Comment 10

[Marc Attinasi]

I'll check this in today

Comment 11

[Marc Attinasi]

Checked into trunk. Thansk Ulrich!

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Marc, a tail call is when the compiler turns "return foo();" into a direct jump
to the start of the foo() function. When foo() returns it effectively returns
from the caller function. It saves cycles and stack space.