I didn't review it yet, but one thing that Gustavo said that is a must and I want to verify will happen is typedefing!
-- Tom. On Tue, Jan 8, 2013 at 10:13 PM, Gustavo Sverzut Barbieri < [email protected]> wrote: > Quick review: > > - Eina_Cow_Ptr should be before given memory to user. > - It would enforce alignment (as user memory could be not aligned). > Eina_Cow_Ptr itself is always aligned because it contains pointers, so it > will not impact what follows. > - It would be less likely to suffer from out-of-bounds writes... that > are VERY common bug :-P > - Please use a macro to get Eina_Cow_Ptr from memory. > - Eina_Cow_Ptr should have magic header... will consume more memory, but > it's a valuable way to avoid errors with users giving invalid pointers > - initial data should be marked as READ ONLY to valgrind or some other > means > > To avoid falling into the same problems as we did with stringshare, please > typedef const void Eina_Cow_Memory and use it. Avoiding confusion where > simple pointers are expected and where actual cow-memory is. > > eina_cow_write() shouldn't take pointer to data, why it does that? just use > the returned value. > > inside eina_cow_write(), if we memcpy(), it's always *data. Remove that > confusing src as it's useless :-) > > eina_cow_commit() is pretty undefined what's the behavior based on the > name... the contents adds it to GC? so commit is "mark as unused"? What if > it's not ref->togc? leaks? Confusing! > eina_cow_commit() exit early if (ref->togc), makes it simpler. > > eina_cow_memcpy() free dst and make dst = src? really? unexpected from > name. > > eina_cow_gc() just handles the last element, logic is confusing :-( > > While I understand the purpose of having eina_cow, I found the API to be > very confusing and misleading. It also seems to be incomplete, yet tries to > to more than the basics without getting the basics right. > Could we get back and model that after some existing concepts, doing the > simple things first, then adding complex stuff as the hash/gc and reuse? > > A good start is: add(), del(), alloc(), free() and write(). Get these right > then follow to extensions such as memcpy() and commit(). > > It would be nice to have some tests to validate what you're trying to > provide and serve as examples as well. > > > > > On Tue, Jan 8, 2013 at 7:17 AM, Enlightenment SVN < > [email protected]> wrote: > > > Log: > > efl: Add eina copy on write infrastructure. > > > > > > Author: cedric > > Date: 2013-01-08 01:17:56 -0800 (Tue, 08 Jan 2013) > > New Revision: 82396 > > Trac: http://trac.enlightenment.org/e/changeset/82396 > > > > Added: > > trunk/efl/src/lib/eina/eina_cow.c trunk/efl/src/lib/eina/eina_cow.h > > trunk/efl/src/tests/eina/eina_test_cow.c > > Modified: > > trunk/efl/src/Makefile_Eina.am trunk/efl/src/lib/eina/Eina.h > > trunk/efl/src/lib/eina/eina_main.c trunk/efl/src/tests/eina/eina_suite.c > > trunk/efl/src/tests/eina/eina_suite.h > > > > Modified: trunk/efl/src/Makefile_Eina.am > > =================================================================== > > --- trunk/efl/src/Makefile_Eina.am 2013-01-08 09:14:35 UTC (rev > 82395) > > +++ trunk/efl/src/Makefile_Eina.am 2013-01-08 09:17:56 UTC (rev > 82396) > > @@ -77,7 +77,8 @@ > > lib/eina/eina_inline_value.x \ > > lib/eina/eina_inline_lock_barrier.x \ > > lib/eina/eina_tmpstr.h \ > > -lib/eina/eina_alloca.h > > +lib/eina/eina_alloca.h \ > > +lib/eina/eina_cow.h > > > > # Will be back for developper after 1.2. > > # lib/eina/eina_model.h > > @@ -140,7 +141,8 @@ > > lib/eina/eina_share_common.h \ > > lib/eina/eina_private.h \ > > lib/eina/eina_strbuf_common.h \ > > -lib/eina/eina_tmpstr.c > > +lib/eina/eina_tmpstr.c \ > > +lib/eina/eina_cow.c > > > > # Will be back for developper after 1.2 > > # lib/eina/eina_model.c \ > > @@ -273,7 +275,8 @@ > > tests/eina/eina_test_str.c \ > > tests/eina/eina_test_quadtree.c \ > > tests/eina/eina_test_simple_xml_parser.c \ > > -tests/eina/eina_test_value.c > > +tests/eina/eina_test_value.c \ > > +tests/eina/eina_test_cow.c > > # tests/eina/eina_test_model.c > > > > tests_eina_eina_suite_CPPFLAGS = \ > > > > Modified: trunk/efl/src/lib/eina/Eina.h > > =================================================================== > > --- trunk/efl/src/lib/eina/Eina.h 2013-01-08 09:14:35 UTC (rev > 82395) > > +++ trunk/efl/src/lib/eina/Eina.h 2013-01-08 09:17:56 UTC (rev > 82396) > > @@ -262,6 +262,7 @@ > > #include "eina_mmap.h" > > #include "eina_xattr.h" > > #include "eina_value.h" > > +#include "eina_cow.h" > > > > #ifdef __cplusplus > > } > > > > Modified: trunk/efl/src/lib/eina/eina_main.c > > =================================================================== > > --- trunk/efl/src/lib/eina/eina_main.c 2013-01-08 09:14:35 UTC (rev > 82395) > > +++ trunk/efl/src/lib/eina/eina_main.c 2013-01-08 09:17:56 UTC (rev > 82396) > > @@ -158,6 +158,7 @@ > > S(value); > > S(tmpstr); > > S(thread); > > + S(cow); > > /* no model for now > > S(model); > > */ > > @@ -199,7 +200,8 @@ > > S(prefix), > > S(value), > > S(tmpstr), > > - S(thread) > > + S(thread), > > + S(cow) > > /* no model for now > > S(model) > > */ > > > > Modified: trunk/efl/src/tests/eina/eina_suite.c > > =================================================================== > > --- trunk/efl/src/tests/eina/eina_suite.c 2013-01-08 09:14:35 UTC > > (rev 82395) > > +++ trunk/efl/src/tests/eina/eina_suite.c 2013-01-08 09:17:56 UTC > > (rev 82396) > > @@ -68,6 +68,7 @@ > > { "Sched", eina_test_sched }, > > { "Simple Xml Parser", eina_test_simple_xml_parser}, > > { "Value", eina_test_value }, > > + { "COW", eina_test_cow }, > > // Disabling Eina_Model test > > // { "Model", eina_test_model }, > > { NULL, NULL } > > > > Modified: trunk/efl/src/tests/eina/eina_suite.h > > =================================================================== > > --- trunk/efl/src/tests/eina/eina_suite.h 2013-01-08 09:14:35 UTC > > (rev 82395) > > +++ trunk/efl/src/tests/eina/eina_suite.h 2013-01-08 09:17:56 UTC > > (rev 82396) > > @@ -57,5 +57,6 @@ > > void eina_test_simple_xml_parser(TCase *tc); > > void eina_test_value(TCase *tc); > > void eina_test_model(TCase *tc); > > +void eina_test_cow(TCase *tc); > > > > #endif /* EINA_SUITE_H_ */ > > > > > > > > > ------------------------------------------------------------------------------ > > Master SQL Server Development, Administration, T-SQL, SSAS, SSIS, SSRS > > and more. Get SQL Server skills now (including 2012) with LearnDevNow - > > 200+ hours of step-by-step video tutorials by Microsoft MVPs and experts. > > SALE $99.99 this month only - learn more at: > > http://p.sf.net/sfu/learnmore_122512 > > _______________________________________________ > > enlightenment-svn mailing list > > [email protected] > > https://lists.sourceforge.net/lists/listinfo/enlightenment-svn > > > > > > -- > Gustavo Sverzut Barbieri > http://profusion.mobi embedded systems > -------------------------------------- > MSN: [email protected] > Skype: gsbarbieri > Mobile: +55 (19) 9225-2202 > > ------------------------------------------------------------------------------ > Master SQL Server Development, Administration, T-SQL, SSAS, SSIS, SSRS > and more. Get SQL Server skills now (including 2012) with LearnDevNow - > 200+ hours of step-by-step video tutorials by Microsoft MVPs and experts. > SALE $99.99 this month only - learn more at: > http://p.sf.net/sfu/learnmore_122512 > _______________________________________________ > enlightenment-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > -- Tom. ------------------------------------------------------------------------------ Master SQL Server Development, Administration, T-SQL, SSAS, SSIS, SSRS and more. Get SQL Server skills now (including 2012) with LearnDevNow - 200+ hours of step-by-step video tutorials by Microsoft MVPs and experts. SALE $99.99 this month only - learn more at: http://p.sf.net/sfu/learnmore_122512 _______________________________________________ enlightenment-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
