Index: TODO =================================================================== diff -u -rdf8c49ffd4cccb5071abd4b7bca0f6d48350c79a -r99c7e7c4351ea0c17971fa41bd5d3c0f5a4c0321 --- TODO (.../TODO) (revision df8c49ffd4cccb5071abd4b7bca0f6d48350c79a) +++ TODO (.../TODO) (revision 99c7e7c4351ea0c17971fa41bd5d3c0f5a4c0321) @@ -5361,16 +5361,26 @@ NsfParameterCacheClassInvalidateCmd() is as efficient as before without to MostGeneralSuperclass optimization (but being complete now) when working on the root classes (an more efficient on subclasses). +- added experimental code feature CYCLIC_MIXIN_ERROR ======================================================================== TODO: -- check other places whether DependentSubClasses() should be used instead of - TransitiveSubClasses() - how should we treat cyclic subclass relationships built via class mixins. (either complain at mixin registration or soft warnings or ignoring - since redundant mixin classes are ignored so far). + if we disallow cycles in class-mixins, we can't do "nx::Object mixin add M" + +- Document/nonnull/resolve NsfRelationClassMixinsSet() +- check other places whether DependentSubClasses() should be used instead of + TransitiveSubClasses() + +- MixinInvalidateObjOrders() is based on TransitiveSubClasses but calls + NsfParameterCacheClassInvalidateCmd() which computes its own set + // MixinInvalidateObjOrders calls NsfParameterCacheClassInvalidateCmd + Some functions use before-lists for cleanup, which is fine. + - should we change "/obj/ info lookup syntax /methodName/" to return obj and method as well? (similar to "info method syntax /methodName/") - we could drop methods::object::info::objectparameter Index: generic/nsf.c =================================================================== diff -u -rdf8c49ffd4cccb5071abd4b7bca0f6d48350c79a -r99c7e7c4351ea0c17971fa41bd5d3c0f5a4c0321 --- generic/nsf.c (.../nsf.c) (revision df8c49ffd4cccb5071abd4b7bca0f6d48350c79a) +++ generic/nsf.c (.../nsf.c) (revision 99c7e7c4351ea0c17971fa41bd5d3c0f5a4c0321) @@ -2279,6 +2279,8 @@ static int TopoSort(NsfClass *cl, NsfClass *baseClass, ClassDirection direction, int withMixinOfs) nonnull(1) nonnull(2); +//#define CYCLIC_MIXIN_ERROR 1 + static int TopoSort(NsfClass *cl, NsfClass *baseClass, ClassDirection direction, int withMixinOfs) { NsfClasses *sl, *pl; @@ -2315,16 +2317,31 @@ NsfCmdList *classMixins = cl->opt && cl->opt->isClassMixinOf ? cl->opt->isClassMixinOf : NULL; for (; classMixins; classMixins = classMixins->nextPtr) { NsfClass *sc = NsfGetClassFromCmdPtr(classMixins->cmdPtr); +#if defined(CYCLIC_MIXIN_ERROR) + if (sc->color == GRAY) { cl->color = WHITE; return 0; } + if (unlikely(sc->color == WHITE && !TopoSort(sc, baseClass, direction, withMixinOfs))) { + if (sc->object.teardown) { + NsfLog(sc->object.teardown, NSF_LOG_WARN, "cycle in the mixin graph list detected for class %s", ClassName(sc)); + } + cl->color = WHITE; + if (cl == baseClass) { + register NsfClasses *pc; + for (pc = cl->order; pc; pc = pc->nextPtr) { pc->cl->color = WHITE; } + } + return 0; + } +#else //if (sc->color == GRAY) { cl->color = WHITE; return 0; } if (unlikely(sc->color == WHITE && !TopoSort(sc, baseClass, direction, withMixinOfs))) { NsfLog(sc->object.teardown, NSF_LOG_WARN, "cycle in the mixin graph list detected for class %s", ClassName(sc)); //cl->color = WHITE; - //f (cl == baseClass) { + //if (cl == baseClass) { // register NsfClasses *pc; // for (pc = cl->order; pc; pc = pc->nextPtr) { pc->cl->color = WHITE; } //} //return 0; } +#endif } } cl->color = BLACK; @@ -8849,13 +8866,17 @@ NsfClass *ncl = (NsfClass *)Tcl_GetHashKey(commandTable, hPtr); /*fprintf(stderr, "Got %s, reset for ncl %p\n", ncl?ClassName(ncl):"NULL", ncl);*/ if (ncl) { + int result; + MixinResetOrderForInstances(ncl); /* * This place seems to be sufficient to invalidate the computed object * parameter definitions. */ /*fprintf(stderr, "MixinInvalidateObjOrders via class mixin %s calls ifd invalidate \n", ClassName(ncl));*/ - NsfParameterCacheClassInvalidateCmd(interp, ncl); + result = NsfParameterCacheClassInvalidateCmd(interp, ncl); + (void) result; // silence compiler + //fprintf(stderr, "MixinInvalidateObjOrders calls NsfParameterCacheClassInvalidateCmd %s => %d\n", ClassName(ncl), result); } } Tcl_DeleteHashTable(commandTable); @@ -25563,13 +25584,20 @@ /* * Clear the cached parsedParam of the class and all its subclasses (the - * result of DependentSubClasses contains the starting class). Furthermore, + * result of DependentSubClasses() contains the starting class). Furthermore, * make a quick check, if any of the subclasses is a class mixin of some * other class. */ dependentSubClasses = DependentSubClasses(cl); +#if defined(CYCLIC_MIXIN_ERROR) + if (dependentSubClasses == NULL) { + //fprintf(stderr, "RAISE ERROR\n"); + return NsfPrintError(interp, "Class heritage graph of %s contains a cycle", ClassName(cl)); + } +#endif + for (clPtr = dependentSubClasses; clPtr; clPtr = clPtr->nextPtr) { NsfClass *subClass = clPtr->cl; @@ -25775,6 +25803,63 @@ return NsfRelationSetCmd(interp, object, relationtype, NULL); } + +static int +NsfRelationClassMixinsSet(Tcl_Interp *interp, NsfClass *cl, Tcl_Obj *valueObj, int oc, Tcl_Obj **ov) { + NsfCmdList *newMixinCmdList = NULL, *cmds; + NsfClasses *subClasses; + NsfClassOpt *clopt = cl->opt; + int i; + + assert(clopt); + + for (i = 0; i < oc; i++) { + if (MixinAdd(interp, &newMixinCmdList, ov[i], cl->object.cl) != TCL_OK) { + CmdListFree(&newMixinCmdList, GuardDel); + return TCL_ERROR; + } + } + if (clopt->classMixins) { + if (clopt->classMixins) RemoveFromClassMixinsOf(cl->object.id, clopt->classMixins); + CmdListFree(&clopt->classMixins, GuardDel); + } + + subClasses = TransitiveSubClasses(cl); + MixinInvalidateObjOrders(interp, cl, subClasses); + + /* + * Since methods of mixed in classes may be used as filters, we have to + * invalidate the filters as well. + */ + if (FiltersDefined(interp) > 0) { + FilterInvalidateObjOrders(interp, subClasses); + } + NsfClassListFree(subClasses); + + /* + * Now register the specified mixins. + */ + clopt->classMixins = newMixinCmdList; + + /* + * Finally, update classMixinOfs + */ + for (cmds = newMixinCmdList; cmds; cmds = cmds->nextPtr) { + NsfObject *nObject = NsfGetObjectFromCmdPtr(cmds->cmdPtr); + if (nObject) { + NsfClassOpt *nclopt = NsfRequireClassOpt((NsfClass *) nObject); + CmdListAddSorted(&nclopt->isClassMixinOf, cl->object.id, NULL); + } else { + NsfLog(interp, NSF_LOG_WARN, + "Problem registering %s as a mixin of %s\n", + ObjStr(valueObj), ClassName(cl)); + } + } + + return TCL_OK; +} + + /* cmd relation::set NsfRelationSetCmd { {-argName "object" -type object} @@ -25995,52 +26080,49 @@ case RelationtypeClass_mixinIdx: { - NsfCmdList *newMixinCmdList = NULL, *cmds; - NsfClasses *subClasses; - for (i = 0; i < oc; i++) { - if (MixinAdd(interp, &newMixinCmdList, ov[i], cl->object.cl) != TCL_OK) { - CmdListFree(&newMixinCmdList, GuardDel); - return TCL_ERROR; - } - } - if (clopt->classMixins) { - if (clopt->classMixins) RemoveFromClassMixinsOf(cl->object.id, clopt->classMixins); - CmdListFree(&clopt->classMixins, GuardDel); - } +#if defined(CYCLIC_MIXIN_ERROR) + Tcl_Obj *oldClassMixinsObj; + NsfClasses *dependentSubClasses; + int result; - subClasses = TransitiveSubClasses(cl); - MixinInvalidateObjOrders(interp, cl, subClasses); + MixinInfo(interp, clopt->classMixins, NULL, 1, NULL); + oldClassMixinsObj = Tcl_GetObjResult(interp); + INCR_REF_COUNT(oldClassMixinsObj); - /* - * Since methods of mixed in classes may be used as filters, we have to - * invalidate the filters as well. - */ - if (FiltersDefined(interp) > 0) { - FilterInvalidateObjOrders(interp, subClasses); + result = NsfRelationClassMixinsSet(interp, cl, valueObj, oc, ov); + if (result != TCL_OK) { + DECR_REF_COUNT(oldClassMixinsObj); + return result; } - NsfClassListFree(subClasses); - /* - * Now register the specified mixins. - */ - clopt->classMixins = newMixinCmdList; + dependentSubClasses = DependentSubClasses(cl); + if (dependentSubClasses == NULL) { + //fprintf(stderr, "RAISE ERROR\n"); - /* - * Finally, update classMixinOfs - */ - for (cmds = newMixinCmdList; cmds; cmds = cmds->nextPtr) { - nObject = NsfGetObjectFromCmdPtr(cmds->cmdPtr); - if (nObject) { - nclopt = NsfRequireClassOpt((NsfClass *) nObject); - CmdListAddSorted(&nclopt->isClassMixinOf, cl->object.id, NULL); - } else { - NsfLog(interp, NSF_LOG_WARN, - "Problem registering %s as a mixin of %s\n", - ObjStr(valueObj), ClassName(cl)); + if (Tcl_ListObjGetElements(interp, oldClassMixinsObj, &oc, &ov) != TCL_OK) { + return TCL_ERROR; } + + result = NsfRelationClassMixinsSet(interp, cl, oldClassMixinsObj, oc, ov); + DECR_REF_COUNT(oldClassMixinsObj); + + if (result != TCL_OK) { + return result; + } + return NsfPrintError(interp, "classes dependent on %s contain a cycle", ClassName(cl)); + + } else { + DECR_REF_COUNT(oldClassMixinsObj); + NsfClassListFree(dependentSubClasses); } +#else + if (NsfRelationClassMixinsSet(interp, cl, valueObj, oc, ov) != TCL_OK) { + return TCL_ERROR; + } +#endif + break; } @@ -26071,8 +26153,11 @@ break; } - } + + /* + * Return on success the final setting + */ NsfRelationSetCmd(interp, object, relationtype, NULL); return TCL_OK; } Index: tests/info-method.test =================================================================== diff -u -r2f793442bb2a7860acc5620811dcafddc43074d3 -r99c7e7c4351ea0c17971fa41bd5d3c0f5a4c0321 --- tests/info-method.test (.../info-method.test) (revision 2f793442bb2a7860acc5620811dcafddc43074d3) +++ tests/info-method.test (.../info-method.test) (revision 99c7e7c4351ea0c17971fa41bd5d3c0f5a4c0321) @@ -913,10 +913,18 @@ Class create M # circular case - B mixin set C - C mixin set B + ? {B mixin set C} "::C" + ? {C mixin get} "" + + #? {C mixin set B} "classes dependent on ::C contain a cycle" + #? {C mixin get} "" ;# make sure, the mixin list of C was reset + + ? {C mixin set B} "::B" + ? {C mixin get} "::B" + ? {B info heritage} "::C ::A ::O ::nx::Object" ? {C info heritage} "::B ::A ::O ::nx::Object" + #? {C info heritage} "::A ::O ::nx::Object" ? {D info heritage} "::A ::O ::nx::Object" # reset @@ -928,9 +936,16 @@ # indirect circular case B mixin set C - C mixin set BB + ? {C mixin get} "" + + ? {C mixin set BB} "::BB" + #? {C mixin set BB} "classes dependent on ::C contain a cycle" + #? {C mixin get} "" + ? {B info heritage} "::BB ::C ::A ::O ::nx::Object" ? {C info heritage} "::BB ::B ::A ::O ::nx::Object" + #? {B info heritage} "::C ::A ::O ::nx::Object" + #? {C info heritage} "::A ::O ::nx::Object" ? {D info heritage} "::A ::O ::nx::Object" # reset @@ -940,22 +955,28 @@ ? {C info heritage} "::A ::O ::nx::Object" ? {D info heritage} "::A ::O ::nx::Object" - M3 mixin set B + ? {M3 mixin set B} ::B ? {A info heritage} "::O ::nx::Object" ? {B info heritage} "::A ::O ::nx::Object" ? {M3 info heritage} "::B ::A ::O ::nx::Object" - A mixin set M3 - + ? {A mixin set M3} "::M3" ? {A info heritage} "::B ::M3 ::O ::nx::Object" ? {B info heritage} "::M3 ::A ::O ::nx::Object" - M3 create m1 + #? {A mixin set M3} "classes dependent on ::A contain a cycle" + #? {A info heritage} "::O ::nx::Object" + #? {B info heritage} "::A ::O ::nx::Object" + + ? {M3 create m1} ::m1 ? {m1 info precedence} "::B ::A ::O ::M3 ::nx::Object" ? {M3 info heritage} "::B ::A ::O ::nx::Object" - B mixin set M3 + ? {B mixin set M3} ::M3 ? {B info heritage} "::M3 ::A ::O ::nx::Object" + + #? {B mixin set M3} {classes dependent on ::B contain a cycle} + #? {B info heritage} "::A ::O ::nx::Object" } #