From: Tom B. (Tehom) <te...@pa...> - 2011-10-25 23:34:34
|
When I implemented Expandfigurationcommand, I didn't know about Chord and GenericChord. "Sets.h" doesn't jump out as containing chord classes, I just stumbled on it recently. I didn't intend to duplicate existing stuff, and I wanted to capture chords better anyways, so I rewrote Expandfigurationcommand to use GenericChord. In doing so, I stumbled across a subtle issue with the AbstractSet hierarchy: AbstractSet tries to call virtual functions in its ctor, via initialise. That doesn't do what you want in derived classes. That's not a compiler bug, it's a deliberate choice C++ made. A base ctor always calls its static versions of virtual functions, not (as one might think) derived versions. So I changed how AbstractSet and its descendants call "initialise". Now Chord and GlobalChord are derived classes, not template instantiations, and initialise is called in their ctors, also in my new derived class's and in NotationChord's. "grep" tells me that that's everything, and everything compiles. I left everything else as it was. I'm aware that my approach merely puts the problem one step further off: deriving from GenericChord now behaves as expected but deriving from Chord etc still doesn't. But the other recommended solutions for C++ are fairly complex, involving helper classes and smart pointers and such. It seemed to me that I'd have risked making more serious problems. And this approach covers everything we're actually doing right now. So I added some explanatory comments for future developers and left it at that. New virtual function initialiseFinish is called by initialise. Its rationale is fairly subtle: the code couldn't just be run in ctors, because we can't guarantee that initialise has already run. New class ChordFromCounterpoint, which is like Chord but also collects notes that started earlier and are still sounding. Improved functionality in ExpandFigurationCommand: * Can handle chords of other than 3 notes. * Can handle chords formed via separate moving voices, no longer just block chords. Tom Breton (Tehom) |
From: Chris C. <ca...@al...> - 2011-11-05 15:38:08
|
[sorry to sit on this so long] On 26 October 2011 00:34, Tom Breton (Tehom) <te...@pa...> wrote: > When I implemented Expandfigurationcommand, I didn't know about Chord and > GenericChord. "Sets.h" doesn't jump out as containing chord classes, I > just stumbled on it recently. Sets.h is rather an old-school filename in Rosegarden terms -- we used to routinely put lots of classes into a single header, and this one is left over from those days. It's also some of the more ambitious and less satisfactory code in Rosegarden, at least in terms of how it is actually used and how fragile it can get. So there are highly likely to be problems there. > In doing so, I stumbled across a subtle issue with the AbstractSet > hierarchy: AbstractSet tries to call virtual functions in its ctor, via > initialise. That doesn't do what you want in derived classes. Having said that -- is this really the case? Looking at the last version before your changes, it looks as if initialise() was only called from the GenericChord (i.e. derived class) ctor, not from AbstractSet (base class) ctor. And GenericChord is not a base class of anything, things like Chord are just template specialisations of it. I agree that the design is wrong, though (AbstractSet really should be purely abstract), and I appreciate this sort of close attention to the logic of things. Let me know if you think I'm misreading it. For what it's worth, I don't see anything (more) wrong about the new code; I've no particular problem with the change. Chris |
From: Tom B. (Tehom) <te...@pa...> - 2011-11-05 16:26:47
|
> Having said that -- is this really the case? Looking at the last > version before your changes, it looks as if initialise() was only > called from the GenericChord (i.e. derived class) ctor, not from > AbstractSet (base class) ctor. And GenericChord is not a base class > of anything, things like Chord are just template specialisations of > it. A specialisation of GenericChord was (still is) used as the base of NotationChord. Which isn't obvious, I agree. I didn't see it until I grepped "GenericChord" through the whole src tree. Thanks for the comments. I appreciate the second pair of eyes. Tom Breton (Tehom) |
From: Chris C. <ca...@al...> - 2011-11-05 16:32:38
|
On 5 November 2011 16:26, Tom Breton (Tehom) <te...@pa...> wrote: > A specialisation of GenericChord was (still is) used as the base of > NotationChord. Aargh. Who wrote this crap? (I did.) Well spotted, that was certainly subtle. I agree that there must be a better fix... whether it's worth the trouble of finding it or not. > grepped "GenericChord" through the whole src tree. Are you familiar with ack? (http://betterthangrep.com/) Very handy for that kind of thing. Chris |
From: Tom B. (Tehom) <te...@pa...> - 2011-11-05 16:45:04
|
> On 5 November 2011 16:26, Tom Breton (Tehom) <te...@pa...> wrote: [...] >> grepped "GenericChord" through the whole src tree. > > Are you familiar with ack? (http://betterthangrep.com/) Very handy > for that kind of thing. Thanks for the pointer. When I wrote "grep", I meant it generically. I actually A-search in emacs' dired buffer. Tom Breton (Tehom) |
From: Chris C. <ca...@al...> - 2011-11-05 16:49:34
|
On 5 November 2011 16:44, Tom Breton (Tehom) <te...@pa...> wrote: > When I wrote "grep", I meant it generically. I thought you might have, but I like to take an opportunity to promote a nice utility. > I actually A-search in emacs' dired buffer. Ah, well -- can't say fairer than that. Chris |