Bug 12717 - CLAMP macro instead of _cairo_restrict_value
Summary: CLAMP macro instead of _cairo_restrict_value
Status: RESOLVED NOTABUG
Alias: None
Product: cairo
Classification: Unclassified
Component: general (show other bugs)
Version: 1.5.1
Hardware: Other All
: medium normal
Assignee: Carl Worth
QA Contact: cairo-bugs mailing list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-10-06 09:22 UTC by Björn Lindqvist
Modified: 2007-10-07 06:40 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Replace _cairo_restrict_value with CLAMP (3.73 KB, patch)
2007-10-06 09:23 UTC, Björn Lindqvist
Details | Splinter Review

Description Björn Lindqvist 2007-10-06 09:22:01 UTC
Here is a patch that replaces _cairo_restrict_value with a CLAMP macro. The macro works exactly like the CLAMP macro in glib. Doing it with a macro is of course faster, more readable (IMHO), and less code to keep around.
Comment 1 Björn Lindqvist 2007-10-06 09:23:00 UTC
Created attachment 11916 [details] [review]
Replace _cairo_restrict_value with CLAMP
Comment 2 Behdad Esfahbod 2007-10-06 11:50:25 UTC
The macro expands its arguments multiple times, so it's not a drop-in replacement.  Our solution to the problem you raise is: first show us that it's a performance bottleneck, then move the function into the header and make it inline.  Please reopen if you do that :)
Comment 3 Daniel Amelang 2007-10-06 12:17:00 UTC
(In reply to comment #2)
> The macro expands its arguments multiple times, so it's not a drop-in
> replacement.  Our solution to the problem you raise is: first show us that it's
> a performance bottleneck, then move the function into the header and make it
> inline.

FWIW, some time last year I also tried "fixing" clamping in cairo, but could never find a realistic test case that made _cairo_restrict_value (or any function that called it) take up a significant portion of the profile (even on ARM9 w/out FPU).

After having said that, though, I hope you aren't discouraged from contributing to cairo. There's still plenty to do. Hope you stick around.
Comment 4 Björn Lindqvist 2007-10-06 19:30:22 UTC
Just to clarify, the intention of the patch is not to fix a non-existent bottleneck. It is to make part of the code more readable. YMMV, but then you are wrong. :) And as you can see in the patch it doesn't matter if the constants 0.0 and 1.0 are expanded multiple times. 

Comment 5 Behdad Esfahbod 2007-10-07 06:40:19 UTC
(In reply to comment #4)
> Just to clarify, the intention of the patch is not to fix a non-existent
> bottleneck. It is to make part of the code more readable. YMMV, but then you
> are wrong. :) And as you can see in the patch it doesn't matter if the
> constants 0.0 and 1.0 are expanded multiple times. 

+#define CLAMP(x, lo, hi)  (((x) > (hi)) ? (hi) : (((x) < (lo)) ? (lo) : (x)))

x is expanded at least twice.  There's no point in fixing what's not broken.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.