[FIX] if cascade cleanup

RESOLVED FIXED in mozilla0.9.7

Status

()

P3
normal
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: drepper, Assigned: attinasi)

Tracking

({memory-footprint})

Trunk
mozilla0.9.7
memory-footprint
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

17 years ago
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).
(Reporter)

Comment 1

17 years ago
Created attachment 49172 [details] [diff] [review]
cleanup patch
(Reporter)

Updated

17 years ago
Keywords: patch, review
(Assignee)

Comment 2

17 years ago
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?
(Reporter)

Comment 3

17 years ago
> 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.
(Assignee)

Comment 4

17 years ago
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

Updated

17 years ago
Target Milestone: --- → mozilla1.0
(Reporter)

Comment 5

17 years ago
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...
Keywords: footprint
(Reporter)

Comment 6

17 years ago
Created attachment 57247 [details] [diff] [review]
Updated patch

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

Updated

17 years ago
Attachment #49172 - Attachment is obsolete: true

Updated

17 years ago
Target Milestone: mozilla1.0 → mozilla1.2
(Reporter)

Comment 7

17 years ago
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.
(Assignee)

Comment 8

17 years ago
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.
Status: NEW → ASSIGNED
OS: Linux → All
Priority: -- → P3
Hardware: PC → All
Target Milestone: mozilla1.2 → mozilla0.9.7

Comment 9

17 years ago
r=rods
(Assignee)

Comment 10

17 years ago
I'll check this in today
Summary: if cascade cleanup → [FIX] if cascade cleanup
(Assignee)

Comment 11

17 years ago
Checked into trunk. Thansk Ulrich!
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.