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 <
no-re...@enlightenment.org> 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
> enlightenment-...@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/enlightenment-svn
>
--
Gustavo Sverzut Barbieri
http://profusion.mobi embedded systems
--------------------------------------
MSN: barbi...@gmail.com
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
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
