This is the mail archive of the cygwin-apps@cygwin.com mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [PATCH] Bigger Chooser Part 3 In Super 3-D: RECTPP


On Sun, 2003-04-06 at 13:01, Gary R. Van Sickle wrote:

> And that adaptation is not in a manner analagous to that happening in e.g.
> HANDLEWrapper*.  xxWrapper are full-blown, honest-to-God classes, with
> destructors and everything, and are not inherited from structs.  Virtual
> functions could be added, one could go nuts, no problems.  Not so with RECTPP.
> 
> And the name "RECTPP" certainly brought to your attention that *something*
> unusual was up, and thus served the very purpose you've claimed on several
> occaisions it doesn't.

I never claimed that RECTPP did not bring it to my attention. I said
that RECTPP is not evocative of your intended use.

> *Both SIDWrapper and HANDLEWrapper are in violation of the "one class == one
> .cc/.h pair" rule, and furthermore are wholly contained in main.cc.  You
> wouldn't let me get away with that.

And I wasn't about to add new files during the troubleshooting of the
security crash in the setup-200303 branch. Besides which, you aren't in
violation of that guideline, and I never suggested you were.

> "RECTWrapper"?  Even though it is a very different beast than other xxxWrappers?
> I really doubt any of us would be satisfied with that situation for any length
> of time.  If I wanted it to be a RECTWrapper, I'd have made it a RECTWrapper.  I
> consciously did not do so because I wanted it to be as lightweight as possible.
> 
> Rob, this is what's frustrating here:
> 
> 1.  You, Max, and myself have now typed enough email to have written a dozen
> RECT wrappers/adapters/whatever.  And there's still no Bigger Chooser in cvs.

And there won't be until we've worked through the review process. 

> 2.  You don't like my multi-include guard, yet only after two iterations of
> patches and several emails have you finally stated the only multi-include guard
> that you would accept.  And what you would accept is not the current pratice in
> the current code.  I.e., you are expecting me to be a mind-reader.

In my first review: "Lastly
CINSTALL is deprecated. We renamed setup from cinstall to setup when we
created the new cvs repository."
"cinstall to setup". Wasn't that clear enough? Apparently not. Telling you that resulted in a GUID!!!!

> 2a.  What matters in a multi-include guard is not how "ugly" it may be, but that
> it prevents multiple-inclusion of the header without colliding with another
> #define.  Nobody ever types it.  Nobody ever looks at it.  It's a one-off (well,
> two-off).  If you demand "SETUP_<filename>_H", fine, whatever, just please don't
> make me try to guess both that and that it's a deal-breaker.

I didn't expect you to guess it. I told you that CINSTALL as the leadin
was deprecated as we had renamed to SETUP.

As for what matters, ugly does matter. 

> 3.  You don't like "RECTPP", but again, only after two patch submissions and
> many emails have you decided to tell me what name you would accept, a name
> suggested by Max, a name which he has now thought better of and disagrees with.
> I also disagree with it, but frankly I see no evidence that my input on this
> subject matters to you.

In the first review I suggested two names I would accept. In deference
to your rejection of both of those I accepted Max's suggestion. You
haven't offered ANY alternatives, you've only defended the choice you
put forward. I haven't said I won't accept other alternatives, but I
have rejected RECTPP.

> 4.  If there is a problem with RECTPP, it's that it isn't a class, but a sort of
> freakshow, inherited from a C struct, public data members flapping in the
> breeze, frankly like nothing I've seen or done before, and certainly not great
> design from a "Design Patterns" perspective.  But on that matter you've not said
> word one, but rather reject it on the multi-include protection #define and its
> *name*!  I simply can't make sense of this.

I queried you on what it was, have accepted it as a valid approach to
make other code clean. Crikey, in the very first review you could have
followed my requests and had this commited.

> 5.  RECTPP is like 0.01% of the Bigger Chooser patch.  Am I going to have to buy
> more bandwidth from my ISP to handle all the patch ping-pong when we get to
> PropSheet.cc?  As I've said before, the changes are considerable.  There are new
> methods and data members; are the *names* going to make the cut?  How many
> iterations before I hit on the ones that will?

Step one: one concept per patch. That will make the review for each part
relatively quick a easy.

> Listen Rob, I didn't come here for a fight.  But I also didn't come here to be a
> mind-reader, or be micro-managed to death, or to spit into the wind.  I came to
> improve Setup.  I've done it before.  My original Big Chooser "uber-patch" does
> it.  The patches I'm trying to submit now will eventually do it, assuming they
> ever make the cut.  My frustration stems not from the amount of work involved in
> doing the changes you request, but from the apparent arbitrariness and
> mind-reading aspects of them.

Gary, I'm getting frustrated too. I'm not here to dictate. I'm here as a
gatekeeper. The points you are claiming I've required "mind reading" on
are clear cut, in the archives, with what I require, and what is
optional in terms of changes. I'm putting the onus on you where there is
a choice, because I am not micro managing you. Where there isn't a
choice, I've said so straight out. 

For instance, to talk about the class naming, I'll accept several
different things. There's one so far that I'm happy with, but that
doesn't preclude others. 

> *Please* tell me how to get this file into cvs Rob.  I need the following:
> 
> 1.  A name for the struct.  I have seen no alternative better than RECTPP, but
> will defer to whatever name you choose.  Just make sure you're happy with it,
> and have thoroughly considered the fact that it is not similar to the xxxWrapper
> classes, and cannot be made to be so.

RECTWrapper will do. Or RECTAdapter. I know the mechanics are different,
but the core intent is the same, afaict.

> 2.  Confirmation that "SETUP_<FILENAME>_H" will not be rejected as a
> multi-include protection #define.

Thats fine.

> 3.  Confirmation that with the above changes, the file will be approved.

If you implement the above two changes, and the method change I
requested in the first review, then I will accept the file.

> 4.  Some indication that we won't have to do this x1000 when we actually get to
> the meat of the Bigger Chooser.

Gary, until I get some more single-concept patches, I can't make any
assertions.

> I will also add some comments wrt virtual functions etc as you mentioned, which
> BTW seems to me to be much more important than either of the other two issues
> we've been on about here.

Thank you.

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

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]