Re: [PATCH 1/4] Move region implementation from mi to dix.
On 22 May 2010 03:05:56 +0200, Soeren Sandmann sandm...@daimi.au.dk wrote: Didn't realize that the hand-rolled changes commit used to be part of this. The comment I have on that part is that it seems to preserve the macro versions of some ops rather than the function versions. Right, I preserved the implementation that was currently being used rather than changing anything about how the code works. That seems like a separate step to me. could just call the pixman equivalent like most of the other Region* API. I'd want some performance numbers before/after this change to verify that nothing 'bad' happens as a result of adding function calls to the region paths. Nothing I did changed how the code worked aside from removing some function calls. -- keith.pack...@intel.com pgp3D8UKCnAG5.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/4] Move region implementation from mi to dix.
On 22 May 2010 03:09:49 +0200, Soeren Sandmann sandm...@daimi.au.dk wrote: - * to miValidateTree. Bob Scheifler changed the representation to be more + * to ValidateTree. Bob Scheifler changed the representation to be more But miValidateTree still exists. That's weird. None of the sed scripts I posted should have done this. I've fixed it manually. -- keith.pack...@intel.com pgp9DoQVyrtrf.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/4] Move region implementation from mi to dix.
On Sat, May 22, 2010 at 12:20 AM, Keith Packard kei...@keithp.com wrote: I've split up the patch which removes the 'mi' prefix from the region function names and the file rename into two patches. If you want the s/mi// rename to be purely mechanical, I think you need another commit first that moves the three declarations from mi/mi.h and miClipSpans from mi/mispans.h into regionstr.h. As it stands now, with the old prototypes being simply deleted, that commit introduces warnings. Looks like you fixed it up two commits later, in Hand-rolled changes to cleanup region macro renaming. Also, Change region implementation from macros to inline functions undoes a couple of changes to comments in region.c (like turning RegionSubtract back into Subtract) that I'm not sure you intended. By the way, Søren, I think an additional patch to use more of pixman instead of duplicating it would be great, but I think Keith's taken the right approach in preserving the existing implementation across this refactoring. Jamey ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/4] Move region implementation from mi to dix.
On Sat, 22 May 2010 11:34:20 -0700, Jamey Sharp ja...@minilop.net wrote: On Sat, May 22, 2010 at 12:20 AM, Keith Packard kei...@keithp.com wrote: I've split up the patch which removes the 'mi' prefix from the region function names and the file rename into two patches. If you want the s/mi// rename to be purely mechanical, I think you need another commit first that moves the three declarations from mi/mi.h and miClipSpans from mi/mispans.h into regionstr.h. As it stands now, with the old prototypes being simply deleted, that commit introduces warnings. Looks like you fixed it up two commits later, in Hand-rolled changes to cleanup region macro renaming. Ok, I may need to regenerate the patch sequence then; it gets a bit messy as I try to clean things up in pieces. -- keith.pack...@intel.com pgp5H24vEXNBf.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/4] Move region implementation from mi to dix.
It looks like renaming miSubtractSpans to SubtractSpans was an accident? With that fixed, this looks perfectly sensible. Reviewed-by: Jamey Sharp ja...@minilop.net Jamey ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/4] Move region implementation from mi to dix.
On Fri, 21 May 2010 13:27:20 -0700, Jamey Sharp ja...@minilop.net wrote: It looks like renaming miSubtractSpans to SubtractSpans was an accident? With that fixed, this looks perfectly sensible. Yeah, the sed script was a bit incautious. I've fixed the sed script and regenerated the branch. In particular, I've split the huge patch into two pieces, the first is purely mechanical and done with the fix-region sed-script, the second patch is the needed fix-ups to make it actually work. When merging this to the server, I would smash those two together to make it bi-sectable, but for review I've split them apart to make it easier to see the two steps. I'll merge the mi - dix change in later. -- keith.pack...@intel.com pgpU3MyPWoajc.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/4] Move region implementation from mi to dix.
On Fri, May 21, 2010 at 3:14 PM, Keith Packard kei...@keithp.com wrote: On Fri, 21 May 2010 13:27:20 -0700, Jamey Sharp ja...@minilop.net wrote: It looks like renaming miSubtractSpans to SubtractSpans was an accident? With that fixed, this looks perfectly sensible. Yeah, the sed script was a bit incautious. I've fixed the sed script and regenerated the branch. You even incorporated a change I hadn't finished writing an e-mail to suggest. Strong work. :-) Seems like the deletions of mi prefixes in region.c should still go in this first commit, though, not in Change region implementation from macros to inline functions. The latter looks kind of broken now. Also, it looks like you ran fix-miregion over fix-miregion. Oops. In particular, I've split the huge patch into two pieces, the first is purely mechanical and done with the fix-region sed-script, the second patch is the needed fix-ups to make it actually work. When merging this to the server, I would smash those two together to make it bi-sectable, but for review I've split them apart to make it easier to see the two steps. Good plan. That does help. Jamey ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/4] Move region implementation from mi to dix.
On Fri, 21 May 2010 15:36:07 -0700, Jamey Sharp ja...@minilop.net wrote: Seems like the deletions of mi prefixes in region.c should still go in this first commit, though, not in Change region implementation from macros to inline functions. The latter looks kind of broken now. Yeah, you're right; deleting the code in the first patch makes things clearer later on. -- keith.pack...@intel.com pgpLY8v49k1Yw.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/4] Move region implementation from mi to dix.
Keith Packard kei...@keithp.com writes: There isn't any reason to have this in mi. Everything except the file rename was done with the included script 'fix-miregion'. Would it be useful to split this commit into two: one that just renames the file and one that is the result of running the script? That way it becomes easier to see what precisely the script actually did, though it does mean there is an intermediate step where the region functions have the wrong name. Also, if we are renaming things anyway, maybe some functions should get moved into the Region name space. For example PointInRegion() could become RegionContainsPoint(). Other than those things, it looks good to me. Søren ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/4] Move region implementation from mi to dix.
Keith Packard kei...@keithp.com writes:
There isn't any reason to have this in mi. Everything except the file
rename was done with the included script 'fix-miregion'.
Didn't realize that the hand-rolled changes commit used to be part
of this. The comment I have on that part is that it seems to preserve
the macro versions of some ops rather than the function versions. For
example, I think it would be better to have this:
void
miRegionInit(RegionPtr pReg, BoxPtr rect, int size)
{
if (rect)
pixman_region_init_with_extents (pReg, rect);
else
pixman_region_init (pReg);
}
as an inline function rather than the version where those two pixman
functions are manually inlined.
Similarly, these functions
RegionUninit
RegionReset
RegionNotEmpty
RegionEmpty - !RegionNotEmpty
could just call the pixman equivalent like most of the other Region*
API.
Søren
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/4] Move region implementation from mi to dix.
Keith Packard kei...@keithp.com writes: There isn't any reason to have this in mi. Everything except the file rename was done with the included script 'fix-miregion'. There is a search-and-replace bug here: - * to miValidateTree. Bob Scheifler changed the representation to be more + * to ValidateTree. Bob Scheifler changed the representation to be more But miValidateTree still exists. In your branch, this happens in the Change region implementation from macros to inline functions commit. Søren ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
