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: [SetupXP] Issue list


Gary R. Van Sickle wrote:
>> Gary,
>>
>> Here is a partial list of issues from your mega-patch.
>>
>
> I still bristle at the "mega" ;-).  43K including the bulk of res.rc ain't
> even *close* to "mega" ;-).

It it if you think about in terms of number of separate concepts included,
instead of byte size :-)

>> * Issue: Drop -r HEAD
>> Please do this ASAP. If you need further evidence for the desirability of
>> this, just look to res.rc, specifically at the way your diff removes my
>> multiline comment about "MS Shell Dlg".
>
> Dude, I tried to do this, but something happened.  I apologize.  Check it
> now, I think it's ok.

Much better, thanks.

>> * Issue: LogFile::Exit
>>         * LogFile.cc (LogFile::exit): Only exit() on error, otherwise
return
>>         to caller.
>>         * LogFile.h (LogFile::exit): Remove noreturn attribute.
>>         * LogSingleton.h (LogSingleton::exit): Remove noreturn attribute.
>>         * main.cc (main): Change some comments.
>> Please either revert these changes, or reopen discussion on this topic.
>
> I maintain the noreturns should be removed on the basis that:
> a. LogXxxx isn't/shouldn't be the long-term program exit() location,

Agree.

> hence they'll have to be removed eventually anyway.
> b. They add nothing in the interim except confusion.

Disagree.
LogSingleton::exit is the medium-term program exit function, and it will be
noreturn in nature until the day the entire function is removed, after all
it's logic has migrated to destructors. In the interim, the noreturn
attribute calls attention to the special nature of this function.

>> * Issue: PropertyPage::OnInit
>> I've suggested a change, and confirmed your understanding of my
suggestion
>> (PreOnInit), but you haven't said whether you think it is a good or bad
>> idea.
>
> I'm not sure yet, that's why ;-).  I think it's good, but I'll have to
look
> into it.

Fair enough.

>> * Issue: IDD_SPLASH res.rc changes
>>         * res.rc (IDD_SPLASH): Move icon.  Change commented-out
>>         "white box" static control to a black invisible one.
>> Please either revert these changes, or reopen discussion on this topic.
>
> Huh, that is an odd size, wonder why I did that.  Alright, but in return
you
> have to tell my why you want me to revert all this stuff in my *local*
copy;
> isn't that why it's local, so's I can play around in my proverbial
sandbox?

OK, clarifying: Please revert it from the local copy you use to generate the
diffs for merging.
But, I cannot see why you would want to have an effectively no-op change in
*any* local copy, anyway.

>> * Issue: OnActivate
>> I'm leaving the final decision up to Robert, then I will distil a patch
>> (unless you want to, in which case, tell me) and submit it for review.
>
> I don't want to unless everybody's happy.

As I said in my last message to Robert on the subject: That was my final
attempt at persuasion. I will now accept Robert's arbitration in order to
get some variant of the change into setup *now*.

Max.


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