There's a good program for finding memory leak :
valgrind
I use it as much as I can (but it slow down your code... a lot!), and it output warning on every "wrong" memory acces, and report memory leaks at the end of the execution.
Well, a very very nice tool!
C memory leak error
Quote:Original post by zaydenam
He actually doesnt know C++, only C, I'm trying to use C++ as little as possible, but my prgram is going to be so big that without classes I would have a nightmare.
So I'll briefly explain the things like classes and objects and anything else that is necessary. He only needs to understand how the program works.
But... but... std::vector is a class too [smile].
Trying to incorporate an implementation of std::vector in your program is not going to aid it's readability.
Quote:Original post by rip-offQuote:Original post by zaydenam
He actually doesnt know C++, only C, I'm trying to use C++ as little as possible, but my prgram is going to be so big that without classes I would have a nightmare.
So I'll briefly explain the things like classes and objects and anything else that is necessary. He only needs to understand how the program works.
But... but... std::vector is a class too [smile].
Trying to incorporate an implementation of std::vector in your program is not going to aid it's readability.
True. If the goal is to keep it simple enough for him to understand, then you should definitely use std::vector. He might not understand every line of code in its implementation, but the nice thing is, he doesn't need to. It's a part of the standard library. And it's a hell of a lot easier to grasp "what std::vector does", than trying to understand someone's C implementation of the same thing. [wink]
(And apart from everything else, I'm guessing he doesn't know C, or any programming language, very well either, if he doesn't know what classes are. And in that case, I'd go for simplicity, which means *not* implementing a C version of std::vector)
First off, use [source][/source] tags for long sections of code instead, to put them in the neat little boxes.
Second, let's address some misunderstandings about how the memory works. There is nothing "leaking" here; instead, what's happening is that we scribble on random memory that doesn't necessarily belong to us.
Problem 1: The compiler has to know the exact size of every *data type* at the time code is compiled. It generates a layout - a structure, hence the keyword 'struct' - of data that is occupied by variables of that type (i.e. instances of that class, when you use a class).
So for example, when you write
On *typical* systems*, this will use 28 bytes of memory for each instance - the data layout consists of "5 * 4 bytes treated as an array of ints named x, followed by 8 bytes treated as a double named y".
Now, what happens when you omit the array dimension?
On typical systems*, again, 'long' and 'int' are both 4 bytes. But what about that array? We didn't say what size it is. That's at odds with the requirement of knowing what the size of an HCF object is.
The evil part: C++ lets this compile, but 'buffer' isn't an array now. It's a pointer, instead. The layout is "4 bytes representing a pointer-to-long named buffer, followed by 4 bytes representing a long named HCFans, followed by 4 bytes representing an int named MAX_FACTORS, followed by 4 bytes representing an int named index" - 16 bytes total. We cannot simply *cause* 'buffer' to be an array of 25 elements later - although we could *point* it at an array *allocation*, which we later clean up.
Problem 2: We never actually do *any* kind of initialization of buffer, though. Let's take a look at the constructor:
Now, look closely at those last two lines. They mention types. That means they are *variable declarations*. It *does not matter* that the variables declared have names that match member names. We are declaring separate variables here, which are local to the constructor. When the constructor runs, the local MAX_FACTORS and buffer variables disappear. And the corresponding members of the object are uninitialized.
Problem 3: DO NOT CALL THE DESTRUCTOR MANUALLY. Sorry for yelling. But you have to understand that, even when it "doesn't do anything", you're not technically allowed to have it run twice for the same object. Destructors are called *automatically* when a local variable goes out of scope - that's the whole point of using them instead of a named "cleanup" function.
Problem 4: Don't include "stdlib.h". There is no such thing in C++. Trying to include both that and <cstdlib> is redundant, anyway. stdlib.h is the C version. <cstdlib> is what you want for C++ code, which you are, de facto, writing as soon as you put the word 'class'. BUT,
Problem 5: Don't artificially pause your programs at the end. Learn to run them properly, instead (i.e. from the command line or via a batch file or by setting a breakpoint at the end of your program in the debugger or ANYTHING BUT pausing it artificially at the end). Consequently, we don't need <cstdlib> either.
Now, let's fix it. You got a few suggestions about how to allocate memory, which would work, but are making things much harder than they need to be. (They also leave you with the responsibility of writing not only a proper destructor, but a proper copy constructor and assignment operator. The fact that you probably don't even know what I'm talking about should be enough to scare you away from this :) )
And then you got some suggestions to make use of the standard library class std::vector instead. This is a great way to manage these kinds of allocations - it wraps up all that work for you, and because it's a class, it's *a data type* - you don't have to muck around with pointers: you can treat the vector just like an array, except that you don't have to decide its size ahead of time.
Of course, if the "maximum size" of 25 had a *real justification*, then you could just build an *actual array* of 25 elements into your HCF struct, by declaring it as 'unsigned long buffer[25];'.
But what I'm going to suggest to you is none of the above. Because all of these approaches are showing totally wrong thinking.
I've found that this is a very common pattern in bad software design, actually: accumulating data for a later all-at-once calculation, when the data will only be read once and written once. In our case, the simple, and in my mind, obvious and correct way of doing things is to just keep a "running total" of the answer.
What will we call this value? Well, right now, there's a data member in the class called 'HCFans', which looks like it was meant to store a result ('HCF answer'). But it never got used, because of how getHCF() was implemented before. A shame, because keeping a running total in such a variable was exactly what should have been done.
But 'HCFans' is a much more cryptic name than it ought to be. First off, it's a bad idea to keep a class' name in the names of its members (i.e. member functions and data members): it doesn't add any information. Do you attach a letter stamp to your letters to mail them? No, you attach a stamp. Of course it's a stamp for a letter; that's what stamps go on. And second, it wouldn't kill us to type "answer" in full.
So, now we can think about what we need:
- We don't need an array, or a "current index in array" value, because we aren't going to store any data.
- We *do* need a member 'answer', which holds the HCF of all the factors that were added so far.
- We don't need 'MAX_FACTORS', because there's nothing for it to limit: the "running total" approach lets us handle as many factors as we like, as long as we don't have an arithmetic overflow.
- We *don't* need a destructor, because we're not going to make any dynamic allocation. As a general rule, any time you would write an empty destructor, AND the class is not going to be derived from, write no destructor at all. (If it would be empty, but the class will be derived from, AND the class will be used polymorphically - i.e. there are 'virtual' member functions - then write an empty destructor, and declare it ALSO to be 'virtual'.)
- We *do* need a constructor, to initialize the answer. What should we initialize it to? Well, the highest common factor of no factors, of course. :) Well, hmm, that doesn't really make any sense, does it? (We *could* meaningfully specify the *lowest common multiple* of no factors, though; it should be a value that, when the LCM operation is applied to it and n, yields n, for any n. That means we should use 1: since 1 divides all n, the LCM of 1 and n is n. But for the HCF, similar logic suggests using infinity, and we can't really represent that neatly.) When we write the constructor, BTW, we'll use an *initializer list* to set the value of 'answer', rather than doing it in the body of the constructor. This is neater, and proper modern C++. :)
- That means we have an *invariant* that the class needs to maintain: there has to be at least one factor in the set of factors for which we are calculating an HCF. The way we make sure of this is to require the first factor *immediately when we create the object*. We do *that* by accepting it as a parameter to the constructor. We *disallow* creation of "default" HCF objects, requiring that the first factor be specified every time we make one.
- We still need a way to add factors, though, because in general we'll want the HCF of more than one number. We used to call this 'addFactor', which I guess is a good enough name.
- We still need a way to get the answer: 'getHCF' again feels like not quite the right name. How about 'getAnswer'?
- Finally, we still need the helper recursive function so that we can update our running total. But this can be *private*, just like the running total itself: outside code should *not* care about this function. It should be using addFactor instead. Also, this function isn't really "bound" to HCF instances; it doesn't use the 'answer' (running total) of any particular instance of the class. Therefore, we can make it 'static'.
This gives us enough information to write the class:
See, when you're doing things properly, the code tends to end up *shorter* than you'd expect. ;)
Second, let's address some misunderstandings about how the memory works. There is nothing "leaking" here; instead, what's happening is that we scribble on random memory that doesn't necessarily belong to us.
Problem 1: The compiler has to know the exact size of every *data type* at the time code is compiled. It generates a layout - a structure, hence the keyword 'struct' - of data that is occupied by variables of that type (i.e. instances of that class, when you use a class).
So for example, when you write
struct Foo { int x[5]; double y;};On *typical* systems*, this will use 28 bytes of memory for each instance - the data layout consists of "5 * 4 bytes treated as an array of ints named x, followed by 8 bytes treated as a double named y".
Now, what happens when you omit the array dimension?
struct HCF { unsigned long buffer[]; unsigned long HCFans; int MAX_FACTORS; int index;};On typical systems*, again, 'long' and 'int' are both 4 bytes. But what about that array? We didn't say what size it is. That's at odds with the requirement of knowing what the size of an HCF object is.
The evil part: C++ lets this compile, but 'buffer' isn't an array now. It's a pointer, instead. The layout is "4 bytes representing a pointer-to-long named buffer, followed by 4 bytes representing a long named HCFans, followed by 4 bytes representing an int named MAX_FACTORS, followed by 4 bytes representing an int named index" - 16 bytes total. We cannot simply *cause* 'buffer' to be an array of 25 elements later - although we could *point* it at an array *allocation*, which we later clean up.
Problem 2: We never actually do *any* kind of initialization of buffer, though. Let's take a look at the constructor:
HCF::HCF() { HCFans = 1; index = 0; const int MAX_FACTORS = 25; unsigned long buffer[MAX_FACTORS]; }Now, look closely at those last two lines. They mention types. That means they are *variable declarations*. It *does not matter* that the variables declared have names that match member names. We are declaring separate variables here, which are local to the constructor. When the constructor runs, the local MAX_FACTORS and buffer variables disappear. And the corresponding members of the object are uninitialized.
Problem 3: DO NOT CALL THE DESTRUCTOR MANUALLY. Sorry for yelling. But you have to understand that, even when it "doesn't do anything", you're not technically allowed to have it run twice for the same object. Destructors are called *automatically* when a local variable goes out of scope - that's the whole point of using them instead of a named "cleanup" function.
Problem 4: Don't include "stdlib.h". There is no such thing in C++. Trying to include both that and <cstdlib> is redundant, anyway. stdlib.h is the C version. <cstdlib> is what you want for C++ code, which you are, de facto, writing as soon as you put the word 'class'. BUT,
Problem 5: Don't artificially pause your programs at the end. Learn to run them properly, instead (i.e. from the command line or via a batch file or by setting a breakpoint at the end of your program in the debugger or ANYTHING BUT pausing it artificially at the end). Consequently, we don't need <cstdlib> either.
Now, let's fix it. You got a few suggestions about how to allocate memory, which would work, but are making things much harder than they need to be. (They also leave you with the responsibility of writing not only a proper destructor, but a proper copy constructor and assignment operator. The fact that you probably don't even know what I'm talking about should be enough to scare you away from this :) )
And then you got some suggestions to make use of the standard library class std::vector instead. This is a great way to manage these kinds of allocations - it wraps up all that work for you, and because it's a class, it's *a data type* - you don't have to muck around with pointers: you can treat the vector just like an array, except that you don't have to decide its size ahead of time.
Of course, if the "maximum size" of 25 had a *real justification*, then you could just build an *actual array* of 25 elements into your HCF struct, by declaring it as 'unsigned long buffer[25];'.
But what I'm going to suggest to you is none of the above. Because all of these approaches are showing totally wrong thinking.
I've found that this is a very common pattern in bad software design, actually: accumulating data for a later all-at-once calculation, when the data will only be read once and written once. In our case, the simple, and in my mind, obvious and correct way of doing things is to just keep a "running total" of the answer.
What will we call this value? Well, right now, there's a data member in the class called 'HCFans', which looks like it was meant to store a result ('HCF answer'). But it never got used, because of how getHCF() was implemented before. A shame, because keeping a running total in such a variable was exactly what should have been done.
But 'HCFans' is a much more cryptic name than it ought to be. First off, it's a bad idea to keep a class' name in the names of its members (i.e. member functions and data members): it doesn't add any information. Do you attach a letter stamp to your letters to mail them? No, you attach a stamp. Of course it's a stamp for a letter; that's what stamps go on. And second, it wouldn't kill us to type "answer" in full.
So, now we can think about what we need:
- We don't need an array, or a "current index in array" value, because we aren't going to store any data.
- We *do* need a member 'answer', which holds the HCF of all the factors that were added so far.
- We don't need 'MAX_FACTORS', because there's nothing for it to limit: the "running total" approach lets us handle as many factors as we like, as long as we don't have an arithmetic overflow.
- We *don't* need a destructor, because we're not going to make any dynamic allocation. As a general rule, any time you would write an empty destructor, AND the class is not going to be derived from, write no destructor at all. (If it would be empty, but the class will be derived from, AND the class will be used polymorphically - i.e. there are 'virtual' member functions - then write an empty destructor, and declare it ALSO to be 'virtual'.)
- We *do* need a constructor, to initialize the answer. What should we initialize it to? Well, the highest common factor of no factors, of course. :) Well, hmm, that doesn't really make any sense, does it? (We *could* meaningfully specify the *lowest common multiple* of no factors, though; it should be a value that, when the LCM operation is applied to it and n, yields n, for any n. That means we should use 1: since 1 divides all n, the LCM of 1 and n is n. But for the HCF, similar logic suggests using infinity, and we can't really represent that neatly.) When we write the constructor, BTW, we'll use an *initializer list* to set the value of 'answer', rather than doing it in the body of the constructor. This is neater, and proper modern C++. :)
- That means we have an *invariant* that the class needs to maintain: there has to be at least one factor in the set of factors for which we are calculating an HCF. The way we make sure of this is to require the first factor *immediately when we create the object*. We do *that* by accepting it as a parameter to the constructor. We *disallow* creation of "default" HCF objects, requiring that the first factor be specified every time we make one.
- We still need a way to add factors, though, because in general we'll want the HCF of more than one number. We used to call this 'addFactor', which I guess is a good enough name.
- We still need a way to get the answer: 'getHCF' again feels like not quite the right name. How about 'getAnswer'?
- Finally, we still need the helper recursive function so that we can update our running total. But this can be *private*, just like the running total itself: outside code should *not* care about this function. It should be using addFactor instead. Also, this function isn't really "bound" to HCF instances; it doesn't use the 'answer' (running total) of any particular instance of the class. Therefore, we can make it 'static'.
This gives us enough information to write the class:
class HCF { // I'll use a typedef so that we aren't constantly writing 'unsigned long' // everywhere. This also makes it so that if we want to change the type // that we use - say for example, to unsigned long long; or say someone else // creates a class that allows for representing really big numbers, and we // want to use that - then we just change the typedef, and everything else is // automatically updated. typedef unsigned long number; number answer; // All the functions of this class are now very simple - so simple that I'll // just implement them directly in the class body... // used to be 'euclidHCF'. The name of the guy who came up with the algorithm // is not important here static number calculate(number a, number b) { if (b == 0) { return a; } // Don't need an "else" here, because we can only get here in the else-case - // otherwise, the return statement would have kicked in. return calculate(b, (a % b)); } public: HCF(number first) : answer(first) {} // Notice we can return void here, now: we'll always be successful, so there // is no need to return an error code. Although, please note that C++ has a // real boolean type, called 'bool', which you should normally use instead of // returning either 1 or 0. void addFactor(number next) { answer = calculate(answer, next); } number getAnswer() { return answer; }};// Then, our calling code looks like:#include <iostream>#include "hcf.h"using namespace std;// Since we aren't using arguments in main, it makes sense to just use the// "plain" form:int main() { // Here's how we pass the "first" factor to the constructor: HCF h1(1000); // Then we can add in the rest: h1.addFactor(432322); h1.addFactor(432295); h1.addFactor(20400); h1.addFactor(5000); h1.addFactor(343223400); h1.addFactor(500143304); h1.addFactor(50423200); h1.addFactor(50003423); h1.addFactor(500042636); h1.addFactor(500042141); h1.addFactor(500042243); h1.addFactor(500042234); h1.addFactor(500042334); h1.addFactor(500042234); h1.addFactor(500042234); h1.addFactor(500042234); h1.addFactor(500042334); h1.addFactor(500042234); h1.addFactor(500042234); h1.addFactor(500042234); h1.addFactor(500042234); h1.addFactor(500042234); h1.addFactor(500042234); h1.addFactor(500042234); cout << h1.getAnswer() << endl; // That's it. No destructor call, no pause, no explicit returning of 0. None // of these things are useful, the first is *completely wrong*, and the // second is exceptionally poor style (albeit common).}See, when you're doing things properly, the code tends to end up *shorter* than you'd expect. ;)
Quote:Original post by zaydenam
He actually doesnt know C++, only C, I'm trying to use C++ as little as possible, but my prgram is going to be so big that without classes I would have a nightmare.
Quote:Original post by Spoonbender
(And apart from everything else, I'm guessing he doesn't know C, or any programming language, very well either, if he doesn't know what classes are.
Speaking from extensive experience as a GDNet member and also as a professional programmer, people who "don't know C++, only C", as a rule, don't know even a tiny fraction of what they think they know of C.
Quote:Or..... std::vector!!!! Where is Zahlman when you need him... :)
Hurrah! I knew he would show up as soon as he smelled poor C/C++ style...
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
