Here's a stab at checking initargs for make-instance and others. With this patch, I get rid of the following errors:
-CHANGE-CLASS.1.11 -MAKE-INSTANCE.ERROR.3 -MAKE-INSTANCE.ERROR.4
Feel free to comment.
The patch seems to produce a result as claimed.
But it's not generic enough. It does not catch, e.g., defgeneric.error.20 nor reinitialize-instance.error.1, which follow the same pattern: supply bogus arguments in initargs. Introducing this patch might create an impression that such cases handled properly always.
Date: Mon, 27 Jul 2009 02:56:21 +0300 From: ville.voutilainen@gmail.com To: armedbear-devel@common-lisp.net Subject: [armedbear-devel] A clos patch for review
Here's a stab at checking initargs for make-instance and others. With this patch, I get rid of the following errors:
-CHANGE-CLASS.1.11 -MAKE-INSTANCE.ERROR.3 -MAKE-INSTANCE.ERROR.4
Feel free to comment.
_________________________________________________________________ Windows Live™ Hotmail®: Celebrate the moment with your favorite sports pics. Check it out. http://www.windowslive.com/Online/Hotmail/Campaign/QuickAdd?ocid=TXT_TAGLM_W...
2009/7/28 Peter Tsenter ptsenter@hotmail.com:
The patch seems to produce a result as claimed. But it's not generic enough. It does not catch, e.g., defgeneric.error.20 nor reinitialize-instance.error.1, which follow the same pattern: supply bogus arguments in initargs. Introducing this patch might create an impression that such cases handled properly always.
Well, I'd rather do these fixes piecemeal than attempt to produce some perfect megapatch that fixes everything clos-related in a single sweep.
First, I'm not against incremental changes, I'm for all the way.
Second, we're not talking about entire clos, but a very specific problem. This problem is not unique for clos, but just heavily used by it.
What I'm afraid of is there is a more fundamental issue here and this patch makes it even more hidden.
BTW, ANSI standard does not allow &aux in generic functions (sec 3.4.2), but this version of clos treats it as a legitimate citizen.
Question: how do you explain why the patch does not catch reinitialize-instance.error.1? How does this case differ from the other 3?
Date: Tue, 28 Jul 2009 01:37:13 +0300 Subject: Re: [armedbear-devel] A clos patch for review From: ville.voutilainen@gmail.com To: ptsenter@hotmail.com CC: armedbear-devel@common-lisp.net
2009/7/28 Peter Tsenter ptsenter@hotmail.com:
The patch seems to produce a result as claimed. But it's not generic enough. It does not catch, e.g., defgeneric.error.20 nor reinitialize-instance.error.1, which follow the same pattern: supply bogus arguments in initargs. Introducing this patch might create an impression that such cases handled properly always.
Well, I'd rather do these fixes piecemeal than attempt to produce some perfect megapatch that fixes everything clos-related in a single sweep.
_________________________________________________________________ Bing™ brings you maps, menus, and reviews organized in one place. Try it now. http://www.bing.com/search?q=restaurants&form=MLOGEN&publ=WLHMTAG&am...
2009/7/28 Peter Tsenter ptsenter@hotmail.com:
What I'm afraid of is there is a more fundamental issue here and this patch makes it even more hidden.
Well, if that's the case, we can always back it out. I understand the concern, but I don't want to be paralyzed by such concerns.
BTW, ANSI standard does not allow &aux in generic functions (sec 3.4.2), but this version of clos treats it as a legitimate citizen. Question: how do you explain why the patch does not catch reinitialize-instance.error.1? How does this case differ from the other 3?
I'm still investigating reinitialize-instance.error.1. It's a bit odd, since it seems to work correctly when the test is copy-pasted to the interpreter. I have no proper explanation for it. If I had, it'd be fixed by now. :)
I urge you to remember that this is a foreign code base for all of us - we may end up taking wrong steps occasionally while searching for the right thing to fix. Be that as it may, I'm not all that worried about the param-check patch. It's very small and relatively isolated.
2009/7/28 Peter Tsenter ptsenter@hotmail.com:
Question: how do you explain why the patch does not catch reinitialize-instance.error.1? How does this case differ from the other 3?
This seems to be related to eval. The make-instance form in reinitialize-instance.error.1 fails correctly when run directly, but when it's done inside eval, it doesn't fail.
Investigations continue..
Btw, I do think that the initarg-check is going to the right direction, because I was able to fix CHANGE-CLASS.ERROR.4 by merely adding a symbolp test to valid-methodarg-p.
I also have a fix for SHARED-INITIALIZE.ERROR.4, which doesn't cause regressions in other tests. That fix is a bit odd, because it requires that initform of (nil foo) is accepted, but something like ('(abc) foo) is not. The thing is, I have to test for (and initarg (not (symbolp initarg))) in std-shared-initialize, which checks that an initarg name must be a symbol (so things like :foo and :bar work) but it can also be nil. The nil case is quite curious. If it's not handled, CLASS-14.1 will fail. That test is
(deftest class-14.1 (let ((c (make-instance 'class-14 nil 'x))) (s1 c)) x)
That seems to be because there's an :initarg nil in the defclass. The class is
(defclass class-14 () ((s1 :initarg nil :reader s1)))
Any comments?
2009/7/28 Ville Voutilainen ville.voutilainen@gmail.com:
That seems to be because there's an :initarg nil in the defclass. The class is (defclass class-14 () ((s1 :initarg nil :reader s1)))
Is this done in order to prevent initializing s1 with an initarg?
2009/7/28 Peter Tsenter ptsenter@hotmail.com:
The patch seems to produce a result as claimed. But it's not generic enough. It does not catch, e.g., defgeneric.error.20
Right.. defgeneric errors indeed seem to be caused by incorrect keyword params on function call. When I find the cause, I can generalize the solution.
armedbear-devel@common-lisp.net
-
Peter Tsenter -
Ville Voutilainen