[cmake-developers] Review request: extract-cmMessenger branch
Michael Scott
michael.scott250 at gmail.com
Tue Feb 2 03:08:18 EST 2016
> Yes. Did you have a close look at the commits? I'm not really sure they are
> correct, and I wonder if you have any thoughts on the first one which
> discusses interface?
I went back and had a closer look at the major changes. I think on the
whole the cmMessenger class is good and a clear improvement over the
previous organisation, however I imagine in its current form it would be
perhaps a bit tricky extending it for other environments (e.g.
displaying messages as GUI modal notifications), which I understood to
be one of the overall aims of the branch (correct me if I've
misunderstood of course though).
I would be tempted to make the following changes to the cmMessenger class:
* Make cmMessenger an abstract class, and have the IssueMessage methods
pure virtual here, then create an extension containing the current
implementations called cmConsoleMessenger or something. The name
cmMessenger suggests a fairly generic concept, but the current
implementation feels geared around a console, though that's just my
thoughts.
* Make the IsMessageTypeVisible and ConvertMessageType methods virtual
and protected, to make it easier to override their behaviour in
extensions of the base class.
* Change printMessagePreamble to return void
* Rename printMessagePreamble to appendMessagePreamble or something to
better reflect what its doing, similar change to printMessageText as well
* Move the different parts to displayMessage into individual methods,
e.g. adding the notes about warning suppression
* Append notes for DEPRECATED_WARNING and DEPRECATED_ERROR message
types, or remove the current notes, to be consistent
I don't really have any other comments for the other commits. I was
wondering if it would be possible to move away from accessing the
cmMessenger class through the cmake / cmMakefile class, but then if we
do want to slot it new implementations easily I imagine the current
organisation would facilitate that quite well so it wouldn't be worth
changing.
Cheers,
Michael
More information about the cmake-developers
mailing list