Re: [Sbcl-devel] [Patch, Bug #310062] (Was: #:(1 2 3) and :(1 2 3))

[Nikodemus S. <nik...@ra...>]

On Sat, Dec 27, 2008 at 6:22 PM, Tobias C. Rittweiler <tc...@fr...> wrote:

> I've worked towards fixing this bug. To fix it properly, some
> refactoring is needed because currently there is no way to determine the
> kind of token (symbol, or number) under consideration.
>
> While refactoring I found out that the refactoring could nicely address
> an old FIXME. Currently, for each explicitly given package qualifier, a
> new string is constructed.
>
> This consing is avoided in the following branch of my git repository:
>
>  http://repo.or.cz/w/sbcl/tcr.git?a=shortlog;h=refs/heads/bug-310062

Thanks!

I gave the branch a *very* quick look. A few things:

* Commit message formatting: please use a style with a
one-line-summary followed by an empty line and the body.
Conterexample: http://repo.or.cz/w/sbcl/tcr.git?a=commit;h=e3d314b11ad5a32726bd31f4a386c041344f9475
. Also, please label whitespace changes as such, starting the first
line of the commit message with "whitespace" or something like that
for easy eyeballing of commit messages.

* This branch is not a clean line of development: things like "Forgot
to change INTERN* in package-data-list." don't belong on a Git branch
proposed for merging. Sometimes things like that end up in CVS since
we're human and screw up occasionally, but as long as the branch is in
Git the mistake can be fixed and history left cleaner.

* Unless there is a specific reason why it is not feasible, *please*
send the relevant patches as attachments, each separate patch leaving
the tree in a working state. If you think the work is easiest to
understand in hindsight as a single patch, that's the way to go. If
you think it's best split into a few separate patches, than so.

Sorry to be so stuck up about this, but patches in email are *really*
preferred for my workflow at least, and having things in email in
easy-to-review chunks that can be applied is much faster for me than
working against your tree directly. (From your Git tree I cannot tell
which revisions are expected to work on their own, and which are not.
In a directly mergeable branch I would expect all to be, but the
aformentioned "oops" commit makes it clear this is not the case.)

> PS: A few notes on what I did in the patches:

These are best made commit messages on the changes. (If you have a
clean line of development to pull the patches from, git format-patch
handles it rigth, if you're manually juggling the patches, adding the
relevant notes to the emails with the patches is fine as well.)

Cheers,

 -- Nikodemus