Dear Robert,
here are some code review comments on ASDF 3.0.2.1.
* Putting :asdf/find-system after :asdf in the recycle looks like it's defeating the purpose; you might as well remove it. * Maybe refactor duplicate-names so it doesn't inherit from system-definition-error ? Or have a function of the same name be called that is defined later to throw the error? * Since we don't work on antique cmucl, maybe we could check whether the :report print-object hack is still needed? * I kind of wanted to prevent the multiplication of .asd files and the eating away of the namespace by using test-asdf/foo systems for new systems; but it's your baby, so your call. Also, an error might not be compatible with putting it in test-asdf.asd. On the other hand, that's a case where define-test-system and/or eval can be used so you don't need a .asd file at all.
* the nodes in the grammar could be renamed to be less confusing and not require a comment.
—♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org The politicians have a most touching faith in technology — that it can make up for any dumb thing the politicians decide to do. — John McCarthy
Faré wrote:
Dear Robert,
here are some code review comments on ASDF 3.0.2.1.
Thank you very much for these comments. They are hugely helpful, and I will act on them.
- Putting :asdf/find-system after :asdf in the recycle looks like it's
defeating the purpose; you might as well remove it.
Will do. I didn't get around to documenting RECYCLE per our conversation on the ticket. I have pushed a revision (3.0.2.2) with documentation, perhaps you could review? In particular, I don't adequately understand the :MIX option, so have left it without documentation.
Would it be reasonable to have DEFINE-PACKAGE have keyword arguments as well as the &rest parameter? The idea would be to make the information an emacs (or other) environment displays to the programmer be more useful.
Another related question: in ENSURE-PACKAGE, there's a case where we ignore names that the programmer has asked us to UNINTERN (when they are :INHERITED). Question: should we be signaling this with a WARNING, in case the user's expressed intent is being violated? Or is this something that must be violated sometimes in order to effectively upgrade (in which case we should leave this here).
- Maybe refactor duplicate-names so it doesn't inherit from
system-definition-error ? Or have a function of the same name be called that is defined later to throw the error?
That seems possible, or perhaps we should just have an ASDF/CONDITIONS package and kick *all* of the conditions up the dependency order? If you want one, just import it or use this package. That seems possibly to cause upgrade issues, though, so I have left this undone.
- Since we don't work on antique cmucl, maybe we could check whether
the :report print-object hack is still needed?
I haven't been testing on either cmucl or abcl, it seems. I will restore those, and maybe we could do this.
Raymond seems like the authority on this.
- I kind of wanted to prevent the multiplication of .asd files and the
eating away of the namespace by using test-asdf/foo systems for new systems; but it's your baby, so your call. Also, an error might not be compatible with putting it in test-asdf.asd. On the other hand, that's a case where define-test-system and/or eval can be used so you don't need a .asd file at all.
Thank you; I will think about making these changes.
- the nodes in the grammar could be renamed to be less confusing and
not require a comment.
Agreed. Also, I'm not sure about the syntax that's used here. There are some uses of @var markup, but it's not clear to me what that signifies. I suspect we haven't used it consistently as the texinfo has accumulated over time.
The manual really needs an end-to-end overhaul, but I don't see myself as having the time to do this in the near future.
best,
r
—♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org The politicians have a most touching faith in technology — that it can make up for any dumb thing the politicians decide to do. — John McCarthy
On Wed, Jul 31, 2013 at 3:45 PM, Robert Goldman rpgoldman@sift.info wrote:
Will do. I didn't get around to documenting RECYCLE per our conversation on the ticket. I have pushed a revision (3.0.2.2) with documentation, perhaps you could review? In particular, I don't adequately understand the :MIX option, so have left it without documentation.
(:MIX PKG1 PKG2 ... PKGn) takes a list of package designators, and (a) behaves like (:USE PKG1 PKG2 ... PKGn) (b) uses (:shadowing-import-from ...) to resolve conflicts in favor of the first found symbol. (c) may still error out if there is a conflict with an explicitly :shadowing-import-from symbol.
Would it be reasonable to have DEFINE-PACKAGE have keyword arguments as well as the &rest parameter? The idea would be to make the information an emacs (or other) environment displays to the programmer be more useful.
DEFINE-PACKAGE does *not* take keyword arguments. It takes a &REST argument that is an alist of clauses, similar to DEFPACKAGE. Your docstring is therefore incorrect.
Also, REEXPORT is subtle in that it exports symbols with the same name as those from the reexported packages, but they need not be the same as those imported from the package, for they may have been shadowed (and the packages may not even be imported?). Maybe a suboptimal name (with usual compatibility issues if you rename).
Another related question: in ENSURE-PACKAGE, there's a case where we ignore names that the programmer has asked us to UNINTERN (when they are :INHERITED). Question: should we be signaling this with a WARNING, in case the user's expressed intent is being violated? Or is this something that must be violated sometimes in order to effectively upgrade (in which case we should leave this here).
My memories are dim, but IIRC, inherited symbols are not considered "interned", unless they are imported (which also happens implicitly when they are exported).
- Maybe refactor duplicate-names so it doesn't inherit from
system-definition-error ? Or have a function of the same name be called that is defined later to throw the error?
That seems possible, or perhaps we should just have an ASDF/CONDITIONS package and kick *all* of the conditions up the dependency order? If you want one, just import it or use this package. That seems possibly to cause upgrade issues, though, so I have left this undone.
I would try to keep the conditions next to code that uses them, introducing each one as late as possible. But that's just me.
—♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org ...so this guy walks into a bar. "The usual, Mr. Descartes?" the barman asked. "I think not," Rene replied, and promptly disappeared.
Thank you very much for the clarifications. I will fix up the documentation accordingly.
Best, R
Sent from my iPad
On Jul 31, 2013, at 17:44, Faré fahree@gmail.com wrote:
On Wed, Jul 31, 2013 at 3:45 PM, Robert Goldman rpgoldman@sift.info wrote:
Will do. I didn't get around to documenting RECYCLE per our conversation on the ticket. I have pushed a revision (3.0.2.2) with documentation, perhaps you could review? In particular, I don't adequately understand the :MIX option, so have left it without documentation.
(:MIX PKG1 PKG2 ... PKGn) takes a list of package designators, and (a) behaves like (:USE PKG1 PKG2 ... PKGn) (b) uses (:shadowing-import-from ...) to resolve conflicts in favor of the first found symbol. (c) may still error out if there is a conflict with an explicitly :shadowing-import-from symbol.
Would it be reasonable to have DEFINE-PACKAGE have keyword arguments as well as the &rest parameter? The idea would be to make the information an emacs (or other) environment displays to the programmer be more useful.
DEFINE-PACKAGE does *not* take keyword arguments. It takes a &REST argument that is an alist of clauses, similar to DEFPACKAGE. Your docstring is therefore incorrect.
Also, REEXPORT is subtle in that it exports symbols with the same name as those from the reexported packages, but they need not be the same as those imported from the package, for they may have been shadowed (and the packages may not even be imported?). Maybe a suboptimal name (with usual compatibility issues if you rename).
Another related question: in ENSURE-PACKAGE, there's a case where we ignore names that the programmer has asked us to UNINTERN (when they are :INHERITED). Question: should we be signaling this with a WARNING, in case the user's expressed intent is being violated? Or is this something that must be violated sometimes in order to effectively upgrade (in which case we should leave this here).
My memories are dim, but IIRC, inherited symbols are not considered "interned", unless they are imported (which also happens implicitly when they are exported).
- Maybe refactor duplicate-names so it doesn't inherit from
system-definition-error ? Or have a function of the same name be called that is defined later to throw the error?
That seems possible, or perhaps we should just have an ASDF/CONDITIONS package and kick *all* of the conditions up the dependency order? If you want one, just import it or use this package. That seems possibly to cause upgrade issues, though, so I have left this undone.
I would try to keep the conditions next to code that uses them, introducing each one as late as possible. But that's just me.
—♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org ...so this guy walks into a bar. "The usual, Mr. Descartes?" the barman asked. "I think not," Rene replied, and promptly disappeared.
-
Faré -
Robert Goldman -
Robert P. Goldman