Closed Bug 817774 Opened 7 years ago Closed 7 years ago

Let nsIntRect not have a nsIntRect static data member, as that kills GDB printing it

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: bjacob, Assigned: bjacob)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
No description provided.
Attachment #687919 - Flags: review?(jmuizelaar)
Attachment #687919 - Flags: review?(jmuizelaar)
Attachment #687919 - Flags: review?(dholbert)
Comment on attachment 687919 [details] [diff] [review]
patch

>+++ b/gfx/src/nsRect.h
>-  static const nsIntRect& GetMaxSizedIntRect() { return kMaxSizedIntRect; }
>+  static const nsIntRect& GetMaxSizedIntRect() {
>+    static nsIntRect r(0, 0, INT_MAX, INT_MAX);
>+    return r;
>+  }

Hmm... Won't this mean that every .cpp that #includes nsIntRect will have its own copy of "static nsIntRect r" ?
Sorry, I meant "...that #includes nsRect.h..."
Comment on attachment 687919 [details] [diff] [review]
patch

...though we could fix that (if it's an issue) by moving the function definition to nsRect.cpp.

Also, we might as well declare "r" as const.

r=me with those fixed  (or with an assurance that static variables defined in header files are smarter than I'm giving them credit for)
Attachment #687919 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #1)
> Comment on attachment 687919 [details] [diff] [review]
> patch
> 
> >+++ b/gfx/src/nsRect.h
> >-  static const nsIntRect& GetMaxSizedIntRect() { return kMaxSizedIntRect; }
> >+  static const nsIntRect& GetMaxSizedIntRect() {
> >+    static nsIntRect r(0, 0, INT_MAX, INT_MAX);
> >+    return r;
> >+  }
> 
> Hmm... Won't this mean that every .cpp that #includes nsIntRect will have
> its own copy of "static nsIntRect r" ?

Sounds like you're confused about the multiple meanings of 'static'.

The 'static' that results in each translation unit having its own copy is file-scope static.

The two 'static' here are different. The first one only means that this method doesn't get a 'this' implicit argument passed to it, but does not make it translation-unit-specific; and then the second 'static' only means that the local 'r' variable is a global variable with the same scope of that function; since that function is not specific to each translation unit, neither is 'r' here.
OK -- I'd thought that static variables (even those inside a function) were translation-unit-specific, but that was an inference based on the way file-scope (and class-scope) static variables behave.  Thanks for clarifying that.

So: -- r=me on the patch as-is, then, though I'd still prefer for 'r' to be declared as const, to be extra-clear about the fact that this method is guaranteed to always return the same thing.
Version: unspecified → Trunk
Good idea to make 'r' const.
http://hg.mozilla.org/integration/mozilla-inbound/rev/97061a550935
Assignee: nobody → bjacob
Target Milestone: --- → mozilla20
First build warning is:
{
In file included from ../../../../../layout/base/nsIPresShell.h:32:
../../../dist/include/nsRect.h:235:36: error: use of undeclared identifier 'INT_MAX'; did you mean 'NS_MAX'?
    static const nsIntRect r(0, 0, INT_MAX, INT_MAX);
                                   ^~~~~~~
                                   NS_MAX
}

from not including <limits.h>.

If we just move the GetMaxSizedIntRect() definition to the nsRect.cpp file (as I suggested for other (bad) reasons in comment 3), that should fix this.
er s/warning/error/
Sorry about that. Fixed. Also, I didn't see comment 9. That would work too, but would prevent inlining when not using link-time optimizations. I don't know if this matters.
That's fine too... Probably doesn't matter much.  It does mean that a lot of places that formerly explicitly-included <limits.h> (including nsRect.cpp) now no longer need to do so, since they'll get it from nsRect.h.  Not a big deal, though.
https://hg.mozilla.org/mozilla-central/rev/e0fce89f6128
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.