Re: [PATCH] Setup.exe in a property sheet
- Original Message - From: "Gary R. Van Sickle" <[EMAIL PROTECTED]> > > It's known as the Big Three Rule. The rule is that if a object needs any > > one of the three (destructor, copy con, assignment) it needs all three. > > The destructor connection is because the _only_ time an object needs a > > non-synthetic destructor is to clean up remote storage/OS objects etc, > > and therefore the same cleanup/management is needed on copy/assignment. > > > > AH, ok, I get it: if you *need* a non-default copy and/or operator=, you also > need a non-default destructor or you're almost assuredly doing something wrong. > That sounds like it would make a good compiler warning. And the converse. If you *need* a non-default destructor, you need the other two, or... That said, if you implement a non-default destructor, implement the other two, as a matter of paranoia and code maintenance. The usual exceptions (i.e. it's just an ABC) aren't worth the headache when something changes badly. > It just goes to prove what I always say, "You learn something every day if > you're not careful". ;-) Always :}. > > This class will be derived > > from > > > std::string when I get gcc to find the $&%^ing header, which will of > > course make > > > it tremendously more functional. > > > > Yes, it seems to be absent from the winsup tree :}. > > > > But I do have it. I've got it in /usr/include/g++-3/string. Same situation > with the cstdlib that inilex.cc needs but gcc can't find, and yet I'm apparently > the only one with problems there. It's fricken driving me insane in the > proverbial membrane. I've got it in /usr too - but it's not in /usr/src/src/ (where my cygwin src tree is rooted). ... > That's just not possible if the modal-ness is all in the constructor. Now, if > you really think it's necessary, it's certainly easy enough to do a constructor > that would take a template ID and a modal/modeless flag, but I don't see how you > can't help but lose the ability to do that stuff inbetween class instance > creation and window+message loop creation, and I think that alone is pretty > important, let alone the error considerations etc. The error consideration is (to me) minor, compared to the programming model & design. The design point you make is good. Leave it as is. > Just to clarify, did you want me to get this diffed against the latest before > you check any of it in? There's only one or two files that are diffed against > non-current HEAD to my knowledge, but I can sure do it, but I'll need a hint as > to how I can do a "cvs update" without bringing back a bunch of stuff that I'll > need to cut right back out again. Or am I SOL and I'll just have to do it by > hand? It's a hand job mate. Update CVS and fix - I have to after other patches go in too ya know :}. Don't worry about diffing against another directory though: just attach the new .cc and .h files as-is and only diff the extant files. Rob
RE: [PATCH] Setup.exe in a property sheet
> From: [EMAIL PROTECTED] > [mailto:[EMAIL PROTECTED]]On Behalf Of Robert Collins [snip] > Can you give me a reference? Last I recall, the comment from MS was the > _beginthread and _endthread leak memory and were deprecated. > The November Platform SDK says this under CreateThread(): "A thread that uses functions from the C run-time libraries should use the beginthread and endthread C run-time functions for thread management rather than CreateThread and ExitThread. Failure to do so results in small memory leaks when ExitThread is called." So it looks like they've not only not deprecated them, they've removed the underscores! ;-) But I don't know, it most certainly would not be the first time MS's right hand didn't know what the left hand was doing, but didn't like the looks of it. [snip] > > Yeah I know. Of course there's currently no copying going on, but > it's on my > > todo list for completeness. I don't understand the destructor > connection > > though...? > > It's known as the Big Three Rule. The rule is that if a object needs any > one of the three (destructor, copy con, assignment) it needs all three. > The destructor connection is because the _only_ time an object needs a > non-synthetic destructor is to clean up remote storage/OS objects etc, > and therefore the same cleanup/management is needed on copy/assignment. > AH, ok, I get it: if you *need* a non-default copy and/or operator=, you also need a non-default destructor or you're almost assuredly doing something wrong. That sounds like it would make a good compiler warning. It just goes to prove what I always say, "You learn something every day if you're not careful". ;-) [snip] > variable length arguments use bitwise copying. Many objects do not > survive bitwise copying (or there would be no need for a copy > constructor). The crash may not occur immediately, but (say) at the > first object access after the destruction of the passed object. Passing > object pointers is less of an issue (for hopefully now clear reasons). > Ok, I can see that that could in at least some cases cause problems. I'll have to think on that more though, it seems to me that in a printf()-type of situation, you're really passing the varargs as const, so if you're casting to const inside the vararg function... oof, I hope I'm not learning again ;-). [snip] > This class will be derived > from > > std::string when I get gcc to find the $&%^ing header, which will of > course make > > it tremendously more functional. > > Yes, it seems to be absent from the winsup tree :}. > But I do have it. I've got it in /usr/include/g++-3/string. Same situation with the cstdlib that inilex.cc needs but gcc can't find, and yet I'm apparently the only one with problems there. It's fricken driving me insane in the proverbial membrane. > > > * To test if something is not needed, comment it out and see if you > get > > > link errors. > > > > Right...? I don't catch your drift. > > You've commented at least one function with "this may not be needed > anymore". > Oh, yep. Last minute hack. We'll get 'er in ship shape before the shape ships. > > > * have you run this through indent? > > > > No (is it that obvious? ;-)), and I have to apologize for that. I > know my > > coding style is rather different *COUGHBETTERHACK* ;-) than the GNU > standard > > style. Is indent still making hash of C++ code? > > Oh yeah. plenty bad, but at least reasonably consistent on many things. > Ugh. Maybe I'll play around with it and see if it can't just adjust the indenting without scrambling everything, and kern the rest by hand. > > > * I don't like the PropertyPage semnatics - Why is Create not the > > > constructor? > ... > > > MyDialogClass dlg(IDD_TEMPLATE, DO_MODAL); > > > > So now in addition to yet another flag you have to deal with, your > dialog lives > > and dies entirely in the constructor, i.e. before the object is even > really > > constructed. You can't, say, construct it and then show/hide it when > you > > wanted, you can't call any members before the box is up (this applies > to both > > modal and modeless), I just don't think it works well or buys you > anything in > > this sort of application. > > Doesn't the same modal issue apply to Create? > No. Say I wanted to do something like this: MyDialog dlg; dlg.LoadDialogWithData(); dlg.CreateModal(); // or dlg.DoModal() or whatever we want to call it That's just not possible if the modal-ness is all in the constructor. Now, if you really think it's necessary, it's certainly easy enough to do a constructor that would take a template ID and a modal/modeless flag, but I don't see how you can't help but lose the ability to do that stuff inbetween class instance creation and window+message loop creation, and I think that alone is pretty important, let alone the error considerations etc. [snip] > > Indeed, but I'm having trouble figuring out how to do it right. > FWICT, I think > > wha
Re: [PATCH] Setup.exe in a property sheet
=== - Original Message - From: "Gary R. Van Sickle" <[EMAIL PROTECTED]> To: <[EMAIL PROTECTED]> Sent: Wednesday, December 19, 2001 7:42 PM Subject: RE: [PATCH] Setup.exe in a property sheet > > Ok, first glance: > > > > You've diffed across versions - please update both the clean dir and > > your working dir for the next patch. Thats a major reason the patch is > > so big. > > > > * please use win32 thread API calls, not _beginthread et al. > > I don't think we can do that, at least not everywhere. The threads call many > CRT functions, and MS warns you not to use CreateThread if you're using the CRT > in your thread. Note that the threads are now "backwards" from what they used > to be - the UI (which IIRC isn't using much if any CRT) now runs entirely in the > main thread, and a few of the do_xxx()'s are split off of that main thread soas > to not block the UI updating/responsiveness. This: A thread that uses functions from the C run-time libraries should use the _beginthread and _endthread C run-time functions for thread management rather than CreateThread and ExitThread. Failure to do so results in small memory leaks when ExitThread is called. Is the reference I remember. Up to you at the end of the day. I think it's a shame mingw still suffers from this. Rob
Re: [PATCH] Setup.exe in a property sheet
- Original Message - From: "Gary R. Van Sickle" <[EMAIL PROTECTED]> > I don't think we can do that, at least not everywhere. The threads call many > CRT functions, and MS warns you not to use CreateThread if you're using the CRT > in your thread. Note that the threads are now "backwards" from what they used > to be - the UI (which IIRC isn't using much if any CRT) now runs entirely in the > main thread, and a few of the do_xxx()'s are split off of that main thread soas > to not block the UI updating/responsiveness. Can you give me a reference? Last I recall, the comment from MS was the _beginthread and _endthread leak memory and were deprecated. > > * All classes with an explicit destructor need copy constructors and > > operator =. (If they aren't used, declare but don't implement). Reason: > > synthesised copy constructors and assignment operators will be wrong. > > (And yes, this is wrong elsewhere in setup too). > > Yeah I know. Of course there's currently no copying going on, but it's on my > todo list for completeness. I don't understand the destructor connection > though...? It's known as the Big Three Rule. The rule is that if a object needs any one of the three (destructor, copy con, assignment) it needs all three. The destructor connection is because the _only_ time an object needs a non-synthetic destructor is to clean up remote storage/OS objects etc, and therefore the same cleanup/management is needed on copy/assignment. > > * varargs and C++ don't mix from what I'm told. (because objects passed > > in lose information). > > I haven't heard that one. As I understand it, you're just pushing bytes onto > the stack, and in the vararg function it's up to you to figure out what they > were supposed to be by whatever means necessary (a cast). I'm not sure why any > code involved in that would care what was being pushed on the stack. variable length arguments use bitwise copying. Many objects do not survive bitwise copying (or there would be no need for a copy constructor). The crash may not occur immediately, but (say) at the first object access after the destruction of the passed object. Passing object pointers is less of an issue (for hopefully now clear reasons). > > It's probably ok for your string class... but I'm > > not sure why it exists? > > Well I'll grant you it's not very fleshed out yet. The idea first and foremost > is to consolidate the LoadString()s and FormatMessage()s into one easy-to-use > place; currently such calls are spread around in several places, and especially > FormatMessage() is a hassle to deal with. This class will be derived from > std::string when I get gcc to find the $&%^ing header, which will of course make > it tremendously more functional. Yes, it seems to be absent from the winsup tree :}. > > * To test if something is not needed, comment it out and see if you get > > link errors. > > Right...? I don't catch your drift. You've commented at least one function with "this may not be needed anymore". > > * have you run this through indent? > > No (is it that obvious? ;-)), and I have to apologize for that. I know my > coding style is rather different *COUGHBETTERHACK* ;-) than the GNU standard > style. Is indent still making hash of C++ code? Oh yeah. plenty bad, but at least reasonably consistent on many things. > > * I don't like the PropertyPage semnatics - Why is Create not the > > constructor? ... > MyDialogClass dlg(IDD_TEMPLATE, DO_MODAL); > > So now in addition to yet another flag you have to deal with, your dialog lives > and dies entirely in the constructor, i.e. before the object is even really > constructed. You can't, say, construct it and then show/hide it when you > wanted, you can't call any members before the box is up (this applies to both > modal and modeless), I just don't think it works well or buys you anything in > this sort of application. Doesn't the same modal issue apply to Create? > > * The propertysheet/propertypage friend relationship would be good to > > have correct. > > Indeed, but I'm having trouble figuring out how to do it right. FWICT, I think > what I really want to do (friend individual member functions to another class) > isn't possible, so I'll probably just friend the entire classes together, which > will at least limit the cross-fertilization to two classes. Sure it's possible. AFAIK it's not possible to only expose particular functions to friends, but it's certainly possible to have a one way friendship. > Ok, sorry. I didn't purposely remove any, and thought they were an automatic CVS > deal anyway. That's ok. The updates are, the creation isn't. Please also add to your new .cc files. > > * The ThreeBar refactoring seems incomplete - it is dependent on the end > > user functions, rather than the other way around. It seems to me that > > the ThreeBar refactor should implement/provide a control but not create > > threads for the install process... > > Well I'm not about to claim th
RE: [PATCH] Setup.exe in a property sheet
> Ok, first glance: > > You've diffed across versions - please update both the clean dir and > your working dir for the next patch. Thats a major reason the patch is > so big. > > * please use win32 thread API calls, not _beginthread et al. I don't think we can do that, at least not everywhere. The threads call many CRT functions, and MS warns you not to use CreateThread if you're using the CRT in your thread. Note that the threads are now "backwards" from what they used to be - the UI (which IIRC isn't using much if any CRT) now runs entirely in the main thread, and a few of the do_xxx()'s are split off of that main thread soas to not block the UI updating/responsiveness. > * All classes with an explicit destructor need copy constructors and > operator =. (If they aren't used, declare but don't implement). Reason: > synthesised copy constructors and assignment operators will be wrong. > (And yes, this is wrong elsewhere in setup too). Yeah I know. Of course there's currently no copying going on, but it's on my todo list for completeness. I don't understand the destructor connection though...? > * varargs and C++ don't mix from what I'm told. (because objects passed > in lose information). I haven't heard that one. As I understand it, you're just pushing bytes onto the stack, and in the vararg function it's up to you to figure out what they were supposed to be by whatever means necessary (a cast). I'm not sure why any code involved in that would care what was being pushed on the stack. > It's probably ok for your string class... but I'm > not sure why it exists? Well I'll grant you it's not very fleshed out yet. The idea first and foremost is to consolidate the LoadString()s and FormatMessage()s into one easy-to-use place; currently such calls are spread around in several places, and especially FormatMessage() is a hassle to deal with. This class will be derived from std::string when I get gcc to find the $&%^ing header, which will of course make it tremendously more functional. > * To test if something is not needed, comment it out and see if you get > link errors. Right...? I don't catch your drift. > * have you run this through indent? No (is it that obvious? ;-)), and I have to apologize for that. I know my coding style is rather different *COUGHBETTERHACK* ;-) than the GNU standard style. Is indent still making hash of C++ code? > * the #if 0...#endifs should go. Delete the code or document why it's > not deleted. They're in the process of going, and I did document this in the ChangeLog (albeit that's not the right place). I'm keeping some of that around until I'm sure I have the logic carried over properly. > * I don't like the PropertyPage semnatics - Why is Create not the > constructor? I don't like constructors that have a significant chance of failing, i.e. that do much more than simply get the instance into a consistent state, and try to avoid them whenever I can. The issue is that when a constructor fails, its only recourse is to throw an exception and let the caller catch it and deal with it (assuming he doesn't want whatever the default abort handler does), and at that point you've more than burned up any convenience you may have gained by combining the constructor and initializer. A separate Create() can still throw an exception if it wants to, but can also return a 'failed' status, which IMO is just as good and easier to catch and take whatever action you deem appropriate. Now in the particular case of these dialog-like things, we've actually got more than one way to initialize them: modal and modeless. A constructor that popped up a modal dialog box, say, sounds like a mess to me; your entire box would have to be something like this: MyDialogClass dlg(IDD_TEMPLATE, DO_MODAL); So now in addition to yet another flag you have to deal with, your dialog lives and dies entirely in the constructor, i.e. before the object is even really constructed. You can't, say, construct it and then show/hide it when you wanted, you can't call any members before the box is up (this applies to both modal and modeless), I just don't think it works well or buys you anything in this sort of application. > * The propertysheet/propertypage friend relationship would be good to > have correct. Indeed, but I'm having trouble figuring out how to do it right. FWICT, I think what I really want to do (friend individual member functions to another class) isn't possible, so I'll probably just friend the entire classes together, which will at least limit the cross-fertilization to two classes. > * please keep CVSID's in source files. They aren't used in the code, but > I find them useful for review. Ok, sorry. I didn't purposely remove any, and thought they were an automatic CVS deal anyway. > * The ThreeBar refactoring seems incomplete - it is dependent on the end > user functions, rather than the other way around. It seems to me that > the ThreeBar refactor should implement/p
Re: [PATCH] Setup.exe in a property sheet
Ok, first glance: You've diffed across versions - please update both the clean dir and your working dir for the next patch. Thats a major reason the patch is so big. * please use win32 thread API calls, not _beginthread et al. * All classes with an explicit destructor need copy constructors and operator =. (If they aren't used, declare but don't implement). Reason: synthesised copy constructors and assignment operators will be wrong. (And yes, this is wrong elsewhere in setup too). * varargs and C++ don't mix from what I'm told. (because objects passed in lose information). It's probably ok for your string class... but I'm not sure why it exists? * To test if something is not needed, comment it out and see if you get link errors. * have you run this through indent? * the #if 0...#endifs should go. Delete the code or document why it's not deleted. * I don't like the PropertyPage semnatics - Why is Create not the constructor? * The propertysheet/propertypage friend relationship would be good to have correct. * please keep CVSID's in source files. They aren't used in the code, but I find them useful for review. * The ThreeBar refactoring seems incomplete - it is dependent on the end user functions, rather than the other way around. It seems to me that the ThreeBar refactor should implement/provide a control but not create threads for the install process... * I'd rather not see _any_ structs - use class's with all public members if needed. * is chooser.o going to be equivalent to choose.o? If so then just fiddle choose.o please. I commit my changes quite frequently so we won't collide much. Lastly, I think it would be a good idea (if possible) to do the refactoring bit-by-bit in future. i.e. factor in the Window class and the threeline progress bar. Then the class conversion for the pages. etc. That just reduces the risk of a huge commit. Rob
