Re: [PATCH] Bigger Chooser 2

[Robert Collins <rbcollins at cygwin dot com>]

This is the first-take review:

rectcc.h contains a class RECTPP. I *think* I posted my preference here
for file names - that is after the class.

So:
RECTPP.h would be the correct name.

Having said that RECTPP is not any of the ok naming conventions for
classes.

A quick recap:
The GNU standards are acceptable, even though I think their application
to C++ is less than effective.
Alternative, capitalised meaningful words. (my preference).

i.e.
rect_pp (GNU)
Rectangle  (best)

within the header, you mix interface and implementation. As I said to
Igor in the last week or so, please don't do this except for one-liners.
(And there while I will accept it, it's not preferred).

void offset(int x, int y); is misnamed IMO. void move(int x, int y);
would be more intuitive to someone reading the code.

Lastly
CINSTALL is deprecated. We renamed setup from cinstall to setup when we
created the new cvs repository.

Ok, onto the main patch.
I don't like the overloading of OnActivate. A query method would be more
appropriate.
i.e.
virtual bool toBeShown() const;

Why are you removing the __attribute__ ((noreturn)) in LogSingleton?

I'm stopping here. There is enough that will need altering, it's
confirmed my opinion.

This won't go in as is.
It also won't go in as a uber-patch. The different things being done
need to be split up, so that we can *sensibly* discuss changing them
without you having to reissue a ~50K patch every time.

Finally:
I don't recall a patch for the OnActivate change coming through. Simply
because there was high latency is not a good reason to make an
*assumption* about how I would have asked you to submit it. And that
assumption you have made is a large contributor to the extra work you
are now being asked to make. 

Rob


-- 
GPG key available at: <http://users.bigpond.net.au/robertc/keys.txt>.

Attachment: signature.asc
Description: This is a digitally signed message part