From: John P. <pet...@cf...> - 2010-11-22 17:30:21
|
Hi, Derek's question about copying SparseMatrix led me to google about ways to automatically keep constructors up-to-date with class members. I didn't really find anything about that, but in doing so I came across one of google's coding style guidelines: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Copy_Constructors "Provide a copy constructor and assignment operator only when necessary. Otherwise, disable them with DISALLOW_COPY_AND_ASSIGN." So apparently they have some macro to declare copy constructor and op= private when they are not needed. At first I thought this was overkill because I assumed that if you provided *any* constructor, the compiler doesn't auto-generate the copy constructor or operator=, but that is wrong. If you ever provide an empty constructor for your class, the copy ctor and op= still get auto-generated... // Class D has a user-defined, non-empty constructor which takes a // (dummy) argument, *and* an empty constructor. This does *not* // prevent auto-generation of other functions class D { public: D() { } D(int) { } }; int main() { D d1; D d2(d1); // auto-generated d1 = d2; // auto-generated return 0; } So... I'm wondering if we should do something similar (disable op= and copy ctor unless needed and explicitly provided, possibly with a macro) in all of our library classes? A cursory examination already revealed some weird stuff: 1.) BoundaryInfo op= implemented but copy ctor not? If you 2.) class LinearSolver has an empty constructor but no copy constructors declared/disabled. It also has a pointer to a Preconditioner object, so a LinearSolver was copied, a shallow copy of the Preconditioner pointer would be made. 3.) DofObject has a public copy constructor but no operator= defined (so probably has an auto-generated op=) and it contains pointer data 4.) PeriodicBoundaries has empty ctor and therefore auto-generated op= and copy ctor. It doesn't contain any additional pointer data though so it's probably OK... for now. -- John |
From: Roy S. <roy...@ic...> - 2010-11-22 17:33:30
|
On Mon, 22 Nov 2010, John Peterson wrote: > So... I'm wondering if we should do something similar (disable op= and > copy ctor unless needed and explicitly provided, possibly with a > macro) in all of our library classes? So a bunch of potential nasty run-time errors turn into compile-time errors? Sounds like a big improvement to me. --- Roy |
From: Boyce G. <gri...@ci...> - 2010-11-22 17:44:57
|
On 11/22/10 12:33 PM, Roy Stogner wrote: > > On Mon, 22 Nov 2010, John Peterson wrote: > >> So... I'm wondering if we should do something similar (disable op= and >> copy ctor unless needed and explicitly provided, possibly with a >> macro) in all of our library classes? > > So a bunch of potential nasty run-time errors turn into compile-time > errors? Sounds like a big improvement to me. FWIW --- the g++ compiler option -Weffc++ can be helpful for tracking down this kind of stuff --- it will emit warnings about classes that violate some of the coding guidelines from Myers' book Effective C++, including one regarding copy constructors and assignment operators. (Because many standard headers violate these guidelines, this option will usually produce a lot of extra compiler noise even for code that follows the Effective C++ guidelines to the letter.) -- Boyce |
From: Roy S. <roy...@ic...> - 2010-11-22 17:59:27
|
On Mon, 22 Nov 2010, Boyce Griffith wrote: > FWIW --- the g++ compiler option -Weffc++ can be helpful for tracking > down this kind of stuff --- it will emit warnings about classes that > violate some of the coding guidelines from Myers' book Effective C++, > including one regarding copy constructors and assignment operators. There's been some discussion here about turning that option on by default in dbg/devel modes and getting libMesh compiling cleanly against it. There's just two problems: The people with the know-how to fix the resulting warnings don't also have enough hours in the day as it is. Various third-party headers we include (mpich2, openmpi, I'm looking at you...) don't all compile cleanly against even the basic gcc warnings; I'd be surprised if they didn't become much much worse with Effective C++ warnings turned on. --- Roy |
From: Boyce G. <gri...@ci...> - 2010-11-22 19:43:40
|
On 11/22/10 12:59 PM, Roy Stogner wrote: > > On Mon, 22 Nov 2010, Boyce Griffith wrote: > >> FWIW --- the g++ compiler option -Weffc++ can be helpful for tracking >> down this kind of stuff --- it will emit warnings about classes that >> violate some of the coding guidelines from Myers' book Effective C++, >> including one regarding copy constructors and assignment operators. > > There's been some discussion here about turning that option on by > default in dbg/devel modes and getting libMesh compiling cleanly > against it. There's just two problems: > > The people with the know-how to fix the resulting warnings don't also > have enough hours in the day as it is. > > Various third-party headers we include (mpich2, openmpi, I'm looking > at you...) don't all compile cleanly against even the basic gcc > warnings; I'd be surprised if they didn't become much much worse with > Effective C++ warnings turned on. I don't use -Weffc++ much for this very reason. An easy approach to dealing with all of the noise is to pipe the output of make through grep -v. My recollection is that it isn't that bad to set this up to separate the wheat from the chaff. -Weffc++ also assumes that you are coding in the fairly specific manner advocated by Myers, which does not apply to all projects. (For the record, I do like his C++ coding rules, even if I don't always follow them.) -- Boyce |
From: Derek G. <fri...@gm...> - 2010-11-22 19:26:05
|
On Nov 22, 2010, at 10:29 AM, John Peterson wrote: > So apparently they have some macro to declare copy constructor and op= > private when they are not needed. At first I thought this was > overkill because I assumed that if you provided *any* constructor, the > compiler doesn't auto-generate the copy constructor or operator=, but > that is wrong. If you ever provide an empty constructor for your > class, the copy ctor and op= still get auto-generated... Yeah - this is what I was nipped by earlier. I did my_matrix = matrix.... and it compiled fine... and just didn't do anything! > 1.) BoundaryInfo op= implemented but copy ctor not? If you Boundary info is tricky. There is a reason there is no copy constructor (I'm partially to blame here as I added the operator=... and went to add the ctor and decided that it couldn't be done). The trouble is with the _mesh reference. If you are making a copy of BoundaryInfo it's probably because you're wanting to copy it to a new mesh... so you don't want to construct the BoundaryInfo object with the old mesh reference (which is what you would do in a straightforward copy constructor). So to work around this I coded up Mesh::operator= to create a new BoundaryInfo object with the incoming mesh... then do bi_new = bi_old using the operator= for BI. Basically doing the construction in two steps. If we wanted to introduce a "copy constructor" that took both the object to be copied and the mesh to use as the _mesh reference.... I think that could work. But it would be a non-standard copy constructor (hmmm.... I suppose it could take that mesh as a pointer with a default argument of NULL and if it is NULL then use the one from the BoundaryInfo object being passed in.... that could work). What do you guys think? BTW - I'm not entirely against the macro... but it has it's own problems. #1 is that you have to remember to put it everywhere. #2 is that you might forget that it is there and try to define operator= and copy ctor... what happens then? Also... adding a bunch of macro stuff like this can be the path to the darkside.... Derek |
From: John P. <pet...@cf...> - 2010-11-22 20:06:03
|
On Mon, Nov 22, 2010 at 1:25 PM, Derek Gaston <fri...@gm...> wrote: > > On Nov 22, 2010, at 10:29 AM, John Peterson wrote: > >> So apparently they have some macro to declare copy constructor and op= >> private when they are not needed. At first I thought this was >> overkill because I assumed that if you provided *any* constructor, the >> compiler doesn't auto-generate the copy constructor or operator=, but >> that is wrong. If you ever provide an empty constructor for your >> class, the copy ctor and op= still get auto-generated... > > Yeah - this is what I was nipped by earlier. I did my_matrix = matrix.... and it compiled fine... and just didn't do anything! > >> 1.) BoundaryInfo op= implemented but copy ctor not? If you > > Boundary info is tricky. There is a reason there is no copy constructor (I'm partially to blame here as I added the operator=... and went to add the ctor and decided that it couldn't be done). The trouble is with the _mesh reference. If you are making a copy of BoundaryInfo it's probably because you're wanting to copy it to a new mesh... so you don't want to construct the BoundaryInfo object with the old mesh reference (which is what you would do in a straightforward copy constructor). So to work around this I coded up Mesh::operator= to create a new BoundaryInfo object with the incoming mesh... then do bi_new = bi_old using the operator= for BI. Basically doing the construction in two steps. I agree that this is a tricky case, and therefore, we shouldn't use operator= as the name of the function you have implemented... operator= should always be just that: assignment with no other unexpected side effects, since that's what someone naturally expects when they say a=b. It may be true that a=b doesn't make sense for BoundaryInfo objects (for the reasons you have mentioned). In that case, we should disable operator= and the function to "sort of" make a copy should be named "assign_boundaryinfo_to_new_mesh()" or something. > BTW - I'm not entirely against the macro... but it has it's own problems. #1 is that you have to remember to put it everywhere. #2 is that you might forget that it is there and try to define operator= and copy ctor... what happens then? Also... adding a bunch of macro stuff like this can be the path to the darkside.... #1 - yep, there's no way around that, and we can't find every case. But I think we should favor code that has the potential to be correct rather than code that generates (possibly) hard-to-diagnose runtime errors. #2 - If you forget that it's there and try to define operator=, you get a compiler error, something along the lines of "cannot be overloaded" so you can deal with it then #2a - I agree that macros are evil, but this one is pretty benign: #define DISALLOW_COPY_AND_ASSIGN(TypeName) \ TypeName(const TypeName&); \ void operator=(const TypeName&); And you place it into the private section of a class via: class D { public: D() { } D(int) { } private: // Macro explicitly prevents copy and assignment by declaring // them private. (Note, you must put the private section in // yourself, it's not part of the macro!) DISALLOW_COPY_AND_ASSIGN(D); }; -- John |
From: Chetan J. <che...@gm...> - 2010-11-22 21:09:43
|
> -----Original Message----- > From: John Peterson [mailto:pet...@cf...] > > #2a - I agree that macros are evil, but this one is pretty benign: > > #define DISALLOW_COPY_AND_ASSIGN(TypeName) \ > TypeName(const TypeName&); \ > void operator=(const TypeName&); Just a curious lurker here. What about a non-macro solution: http://www.boost.org/doc/libs/1_43_0/boost/noncopyable.hpp Chetan > And you place it into the private section of a class via: > > class D > { > public: > D() { } > D(int) { } > > private: > // Macro explicitly prevents copy and assignment by declaring > // them private. (Note, you must put the private section in > // yourself, it's not part of the macro!) > DISALLOW_COPY_AND_ASSIGN(D); > }; > > -- > John |
From: John P. <pet...@cf...> - 2010-11-22 22:33:25
|
On Mon, Nov 22, 2010 at 3:09 PM, Chetan Jhurani <che...@gm...> wrote: >> -----Original Message----- >> From: John Peterson [mailto:pet...@cf...] >> >> #2a - I agree that macros are evil, but this one is pretty benign: >> >> #define DISALLOW_COPY_AND_ASSIGN(TypeName) \ >> TypeName(const TypeName&); \ >> void operator=(const TypeName&); > > > Just a curious lurker here. What about a non-macro solution: > > http://www.boost.org/doc/libs/1_43_0/boost/noncopyable.hpp This is a nice solution but IMHO changing the inheritance hierarchy is a little intrusive just to get two little functions disabled! -- John |
From: Derek G. <fri...@gm...> - 2010-11-22 22:34:33
|
On Nov 22, 2010, at 3:32 PM, John Peterson wrote: > This is a nice solution but IMHO changing the inheritance hierarchy is > a little intrusive just to get two little functions disabled! I concur. Derek |
From: Chetan J. <che...@gm...> - 2010-11-22 22:40:15
|
> -----Original Message----- > From: jwp...@gm... [mailto:jwp...@gm...] On Behalf Of John Peterson > > On Mon, Nov 22, 2010 at 3:09 PM, Chetan Jhurani > <che...@gm...> wrote: > >> -----Original Message----- > >> From: John Peterson [mailto:pet...@cf...] > >> > >> #2a - I agree that macros are evil, but this one is pretty benign: > >> > >> #define DISALLOW_COPY_AND_ASSIGN(TypeName) \ > >> TypeName(const TypeName&); \ > >> void operator=(const TypeName&); > > > > > > Just a curious lurker here. What about a non-macro solution: > > > > http://www.boost.org/doc/libs/1_43_0/boost/noncopyable.hpp > > This is a nice solution but IMHO changing the inheritance hierarchy is > a little intrusive just to get two little functions disabled! Makes good sense. I was thinking in terms of changing a few classes only and not the whole codebase. Chetan > > -- > John |
From: Roy S. <roy...@ic...> - 2010-11-22 22:43:52
|
The boost suggestion reminds me of something I meant to bring up on Friday (before work and vacation packing distracted me): we need to use some shared_ptr classes. Vikram's doing some work with the ErrorMap, and looking at that again reminds me how horrible it is to force manual memory management into non-performance-critical sections of user code. I'd originally planned on writing a SharedPtr wrapper that would hook to C++0x or to optional boost or to an internal fallback... but maybe it's time to bite the bullet and just introduce boost as a libMesh dependency? Other opinions would be appreciated. --- Roy On Mon, 22 Nov 2010, Chetan Jhurani wrote: >> -----Original Message----- >> From: John Peterson [mailto:pet...@cf...] >> >> #2a - I agree that macros are evil, but this one is pretty benign: >> >> #define DISALLOW_COPY_AND_ASSIGN(TypeName) \ >> TypeName(const TypeName&); \ >> void operator=(const TypeName&); > > > Just a curious lurker here. What about a non-macro solution: > > http://www.boost.org/doc/libs/1_43_0/boost/noncopyable.hpp > > Chetan > > >> And you place it into the private section of a class via: >> >> class D >> { >> public: >> D() { } >> D(int) { } >> >> private: >> // Macro explicitly prevents copy and assignment by declaring >> // them private. (Note, you must put the private section in >> // yourself, it's not part of the macro!) >> DISALLOW_COPY_AND_ASSIGN(D); >> }; >> >> -- >> John > > > > ------------------------------------------------------------------------------ > Increase Visibility of Your 3D Game App & Earn a Chance To Win $500! > Tap into the largest installed PC base & get more eyes on your game by > optimizing for Intel(R) Graphics Technology. Get started today with the > Intel(R) Software Partner Program. Five $500 cash prizes are up for grabs. > http://p.sf.net/sfu/intelisp-dev2dev > _______________________________________________ > Libmesh-devel mailing list > Lib...@li... > https://lists.sourceforge.net/lists/listinfo/libmesh-devel > |
From: John P. <pet...@cf...> - 2010-11-22 22:51:49
|
On Mon, Nov 22, 2010 at 4:43 PM, Roy Stogner <roy...@ic...> wrote: > > The boost suggestion reminds me of something I meant to bring up on > Friday (before work and vacation packing distracted me): we need to > use some shared_ptr classes. Vikram's doing some work with the > ErrorMap, and looking at that again reminds me how horrible it is to > force manual memory management into non-performance-critical sections > of user code. > > I'd originally planned on writing a SharedPtr wrapper that would hook > to C++0x or to optional boost or to an internal fallback... but maybe > it's time to bite the bullet and just introduce boost as a libMesh > dependency? Other opinions would be appreciated. Last time I checked, we have shared_ptr.hpp in the contrib/boost directory. Just #include and go for it. Well, you might want to get the latest version, but AFAIK it's just one header file. -- John |
From: Derek G. <fri...@gm...> - 2010-11-22 22:53:30
|
On Nov 22, 2010, at 3:43 PM, Roy Stogner wrote: > I'd originally planned on writing a SharedPtr wrapper that would hook > to C++0x or to optional boost or to an internal fallback... but maybe > it's time to bite the bullet and just introduce boost as a libMesh > dependency? Other opinions would be appreciated. We were actually looking at this a few weeks ago when we needed a specific sorting algorithm that they have in boost. What did we finally decide? We rewrote the sorting algorithm. Why did we do that? Because it was simpler to rewrite the algorithm (only like 10 lines of code) than it was to deal with the hundreds (yes really! We actually ran the compiler and had it generate the dependencies so we could see just how much of Boost we would need to pull in so we could have that sorting algorithm) of header file dependencies that would have been pulled in! Personally, I'm not a fan of Boost. Sometimes it provides well modularized useful functionality... but more often it seems as if it's a computer science project that can't possibly be understood by any normal human (yes, Roy, that _does_ mean that you're not a _normal_ human ;-). At any rate... this isn't a vote against having it be a dependency.... just a cautionary tale about Boost in general. How would we handle the dependency? Would you copy all of Boost (which is a lot) into /contrib? Or would you make the user put it on their system somewhere? I really don't like the latter idea... I've always thought it was cool that you could checkout and build libMesh without any dependencies at all (if you just want to do serial solves on one CPU).... we should think pretty hard about this before we make that plunge. Derek |