On Sat, Mar 15, 2014 at 9:09 PM, Jean-Claude Beaudoin jean.claude.beaudoin@gmail.com wrote:
Here is, as previously mentioned, the git format-patch with the modifications needed to port current ASDF head to MKCL 1.1.8 and later. This has been tested on Linux Ubuntu 12.04 and MKCL 1.1.8 where it passes all 51 tests of the current ASDF test-lisp without any failure.
Hi, Jean-Claude,
thanks a lot for your patch. Congratulations! It mostly looks great. However, here some issues. Can you fix them and resubmit? Since only you can test with MKCL, I'd rather you do it.
1- Why the &allow-other-keys in + (defmethod initialize-instance :before ((o operation) &key &allow-other-keys) + (defmethod initialize-instance :before ((o non-propagating-operation) &key &allow-other-keys) + (defmethod reinitialize-instance :after ((self wild-module) &key &allow-other-keys) + (defmethod shared-initialize :after ((o operation) (slot-names t) &key &allow-other-keys) (defmethod initialize-instance :after ((plan filtered-sequential-plan)
That looks like a big bug in MKCL if you require that flag: it makes the function accept all keys, when if should only accept keys that are explicitly defined in *one of the methods*. Please fix MKCL.
2- I explicitly recommend against putting slots in operation. This is not well-supported by ASDF, and the constraints are not well-documented and subject to change in the future. Basically, the initargs may or may not be passed on to children operations, and are not taken in consideration by the table that remembers whether an action was done. Big no no. Instead, please add slots to system if you must, or just use new operations. The existing slots are there for backward-compatibility only, and I'd like to get rid of them.
3- No, it's NOT OK to rename bundle-system to bundled-system. That's not backward-compatible, and such a change requires maintainer approval. The maintainer is Robert.
4- Please follow the usual CL style for parens. No C-style dangling parens, please.
5- For the sake of ABCL, the test for mingw32 mingw64 should be at runtime, using (if (featurep '(:or :mingw32 ...)) or maybe adding that to the os-unix-p conditional branch.
6- How come you don't need the uiop-library-file on MKCL? Sounds dubious.
7- Please no #-(and) anywhere
8- in run-program, this looks very bad on non-mkcl implementations: #+os-windows + #+mkcl (list "cmd" '#:/c command)
9- Can you run the tests with one other implementation of your choice before you send, just to check you haven't broken anything obvious as above?
10- Also, you can remove this comment: + ;; both combined
PS: I saw and fixed slight bug in load-fasl-op, please update to the latest ASDF.
Robert, can you handle the merge when JCB comes with a cleaned up patch?
—♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org They laughed at Columbus, they laughed at Fulton, they laughed at the Wright brothers. But they also laughed at Bozo the Clown. — Carl Sagan