Re: First attempt at new accidentals in git

[Rune Zedeler]

Han-Wen Nienhuys skrev:

> what's
>
>     midt i at ændre pitch og tilføje pitchclass

Huh?!
I cannot come up with other explanations than that it must be a bug in 
git 1.4 that has been fixed in 1.5 because I am unable to reproduce.
I made some changes in some files and added some new files, and 
committed to git.
After the commit I realised that I had written the comment in danish, so 
I did a git reset and a new git commit with an english comment.
It seems like the reset did not correctly remove the newly added files - 
it only reverted the changed files.
"midt i at ændre pitch og tilføje pitchclass" is danish for "in the 
middle of changing pitch and adding pitchclass" - the comment on the 
commit that was done 6 minutes later.

> > +      if (g!=this)
>
> style: spaces around !=
>
> I think this was discussed on the list before that this only handles
> simple minded cycles? If yes, please add a comment.

Yep. Of course I will either complete the change (i.e. remove the 
"inner" contexts and make convert-ly-rules) or leave this change out 
when I push to master.

> > +/*
> > +FIXME!!!
> > +Does it work with simple smobs when it contains a reference to a pitchclass_?
> > +*/
>
> Yes, however, you must make sure that the Pitchclass* does not come
> from a SCM, ie.
>
>   pitchclass_ = unsmob_pitchclass (scm_obj)  // wrong, pitchclass_
>   // may be deleted if scm_obj becomes unreachable
>
>   pitchclass_ = new Pitchclass (*unsmob_pitchclass (scm_obj))  // wrong

Eh, do you mean that the lower possibility is /right/ (not wrong)...?

> > +class Pitch : public Pitchclass
> >  {
> >  private:
> >    int octave_;
> >    Pitch negated () const;
> >    string to_string () const;
> > +  SCM virtual_smobbed_copy () { // AAARGH!
> > +    return smobbed_copy ();
> > +  }
>
> >    DECLARE_SIMPLE_SMOBS (Pitch);
>
> This looks bad. You have to remove Pitch as a smobtype, and then
> create an unsmob_pitch, which does
>
>   dynamic_cast<Pitch*> (unsmob_pitchclass (obj))

That won't work with simple smobs.
If you call Pitchclass::smobbed_copy() on an object of type Pitch then 
the smob will only contain the fields of Pitchclass, and therefore 
Pitchclass::unsmob will always return an object of class Pitchclass - 
even if the original object was of class Pitch.

I really don't like moving to complex smobs for something as simple as a 
pitch.
What is the problem with having both Pitch and Pitchclass as smobtypes?

> Then, double check that all relevant functions distinguish between
> unsmob_pitch()  and unsmob_pitchclass() correctly.

Hmm. So what you say is that the programmer should basically

Pitchclass * pc = unsmob_pitch (scm);
if (!pc) pc = unsmob_pitchclass (scm);

everytime he needs to unsmob a scm that might be a pitch or might be a 
pitchclass? That seems a bit cumbersome.

> You should also modify the print routine for the Pitch(class) smob so that it
> will print
>
>  #<Pitch c''>
>
> vs
>
>  #<Pitchclass c>

This will afaics require the Pitchclass implementation to know about the 
Pitch implementation - which is very bad oo style.

(The pitchclass print_smob should check whether the SCM contains a Pitch 
and if so, call Pitch::print_smob).
IMO the right thing would be to add a virtual method that does the 
printing, and then print_smob can call that method.

> > +INSTANTIATE_COMPARE (Pitchclass, Pitchclass::compare);
>
> that puzzles me: how can you compare pitchclasses, which are
> essentially cyclic?

My idea was to compare them based on the position in the key-signature:

c f bes es ... g d a ...

That way one could just sort the list of pitchclasses before generating 
the key-signature accidentals. Ofcourse this would need some sort of 
comment.

-Rune