Ideas for how to improve code
Author
[font=UsherwoodStd-Medium][font=UsherwoodStd-Medium]I agree with that and am posting here to ask for some ideas on how to improve the code here --[/font][/font]
Consistency is nice, but as long as everything is relatively readable, it should be the least of one's concerns, IMHO.
I haven't looked very hard. Without spending a long time trying to figure out it's purpose, it's hard to be sure if it's fit for purpose. Also, a lot of it is headers without any implementation, so what I'm not sure what you're hoping to get out of this.
But...
I don't see any tests. bug_count(code without tests) >= bug_count(same code with tests). So that's one easy way of improving the code.
The "[color=#000000]malloc_never_null" function is also extremely weird, especially since it's used in a context where you know there's a maximum buffer size of 255 (which is peanuts for almost any system).
[color=#000000]I counted a grand total of 2 asserts
I haven't looked very hard. Without spending a long time trying to figure out it's purpose, it's hard to be sure if it's fit for purpose. Also, a lot of it is headers without any implementation, so what I'm not sure what you're hoping to get out of this.
But...
I don't see any tests. bug_count(code without tests) >= bug_count(same code with tests). So that's one easy way of improving the code.
The "[color=#000000]malloc_never_null" function is also extremely weird, especially since it's used in a context where you know there's a maximum buffer size of 255 (which is peanuts for almost any system).
[color=#000000]I counted a grand total of 2 asserts
/*
* Like new, we want to guarantee that we NEVER
* return NULL. Loop until there is free memory.
*
*/
static char* malloc_never_null (size_t const b)
{
char *p;
do {
p = static_cast<char*>(malloc(b));
} while ( p == NULL );
return p;
}This is just wrong, you should feel ashamed.. the reason new never returns a null pointer is because it throws an exception if it can't allocate the requested amount of memory.
Unless you disable exceptions in your specific compiler, then new can of course return null pointers (depending on implementation specifics).
This is just wrong, you should feel ashamed.
There's no shame in doing something incorrectly.
the reason new never returns a null pointer is because it throws an exception if it can't allocate the requested amount of memory.
Unless you disable exceptions in your specific compiler, then new can of course return null pointers (depending on implementation specifics).
To expand on this, [font=courier new,courier,monospace]new [/font]can be made to return [font=courier new,courier,monospace]NULL[/font]/[font=courier new,courier,monospace]nullptr [/font]upon failure if you use [font=courier new,courier,monospace]nothrow[/font]. In which case, [font=courier new,courier,monospace]new [/font]may return [font=courier new,courier,monospace]NULL[/font]. If there isn't any available memory when you request it, there probably won't be any the next time you request it 700 nanoseconds later, and there isn't much sense in getting stuck in a potentially infinite loop.
I'm curious as to why you're using [font=courier new,courier,monospace]malloc [/font]instead of [font=courier new,courier,monospace]new[/font].
While edd[sup]2[/sup] has a point about consistency not being critical to having a working product, I will say that I would be horribly frustrated if a project was inconsistent in its conventions.
This line (from MarshallingFunctions.hh):
(boolRep != 0) ? bl = true : bl = false;is technically valid, but it goes against every normal use of the ternary operator. If you wanted it to be consistent with how the other 99.99% of the world programs, it would be:
bl = boolRep != 0;or at most:
bl = (boolRep != 0) ? true : false;If you really wanted to do it that way though, you could also do
if (boolRep != 0) b1 = true; else b1 = false;which is slightly common (but depends on your coding conventions), but I'd say by far the most common are the first two I mentioned.
[edit]
I will commend you on seeking to improve your code. It's not always easy to ask for a critical review.
There's no shame in doing something incorrectly.
There's no good reason to use malloc in C++ anyway, especially if you're about to use it incorrectly.
If you really want "performance" (PLEASE PROFILE FIRST), then you can use the OS-specific HeapRealloc or POSIX realloc APIs and hide them behind an overloaded new operator.
[quote name='Mihai Moldovan' timestamp='1335828424' post='4936216']
This is just wrong, you should feel ashamed.
There's no shame in doing something incorrectly.
[/quote]
+1. The only shame lies in not learning why it was incorrect.
Author
[quote name='Mihai Moldovan' timestamp='1335828424' post='4936216']
the reason new never returns a null pointer is because it throws an exception if it can't allocate the requested amount of memory.
Unless you disable exceptions in your specific compiler, then new can of course return null pointers (depending on implementation specifics).
To expand on this, [font=courier new, courier, monospace]new [/font]can be made to return [font=courier new, courier, monospace]NULL[/font]/[font=courier new, courier, monospace]nullptr [/font]upon failure if you use [font=courier new, courier, monospace]nothrow[/font]. In which case, [font=courier new, courier, monospace]new [/font]may return [font=courier new, courier, monospace]NULL[/font]. If there isn't any available memory when you request it, there probably won't be any the next time you request it 700 nanoseconds later, and there isn't much sense in getting stuck in a potentially infinite loop.
I'm curious as to why you're using [font=courier new, courier, monospace]malloc [/font]instead of [font=courier new, courier, monospace]new[/font].
[/quote]
I picked that code up from someone on line and haven't paid enough attention to it yet. It probably should use new.
While edd[sup]2[/sup] has a point about consistency not being critical to having a working product, I will say that I would be horribly frustrated if a project was inconsistent in its conventions.
[/quote]
I agree. It would be too arbitrary otherwise. What I may do is adopt one naming convention for my library and the generated code and another naming convention for the executables. I'm not really comfortable with lower case and underscores exclusively or all camel case. I'm going to work on the convention for the library first.
This line (from MarshallingFunctions.hh):
(boolRep != 0) ? bl = true : bl = false;
is technically valid, but it goes against every normal use of the ternary operator. If you wanted it to be consistent with how the other 99.99% of the world programs, it would be:
bl = boolRep != 0;
or at most:
bl = (boolRep != 0) ? true : false;
If you really wanted to do it that way though, you could also do
if (boolRep != 0) b1 = true; else b1 = false;
which is slightly common (but depends on your coding conventions), but I'd say by far the most common are the first two I mentioned.
[/quote]
I think I'll go with this one:
bl = (boolRep != 0) ? true : false;
This is a small thing, but these things add up.
Author
Consistency is nice, but as long as everything is relatively readable, it should be the least of one's concerns, IMHO.
I haven't looked very hard. Without spending a long time trying to figure out it's purpose, it's hard to be sure if it's fit for purpose. Also, a lot of it is headers without any implementation, so what I'm not sure what you're hoping to get out of this.
There's an archive here that has all of the files.
But...
I don't see any tests. bug_count(code without tests) >= bug_count(same code with tests). So that's one easy way of improving the code.
The "[color=#000000]malloc_never_null" function is also extremely weird, especially since it's used in a context where you know there's a maximum buffer size of 255 (which is peanuts for almost any system).
[color=#000000]I counted a grand total of 2 asserts
[/quote]
You got it right. (There's more than two but they are in the segmented_array code by Leigh Johnston that I ship with the archive.) Perhaps you could show an example of where you would add an assert.
Perhaps you could show an example of where you would add an assert.
As one example, you could be pedantic by changing (from MarshallingFunctions.hh)
inline
void
CstringCount (::cmw::Counter& cntr, char const* cstr)
{
marshalling_integer slen(::strlen(cstr));
slen.CalculateMarshallingSize(cntr);
cntr.Add(slen.value);
}to:
inline
void
CstringCount (::cmw::Counter& cntr, char const* cstr)
{
assert(cstr != NULL);
marshalling_integer slen(::strlen(cstr));
slen.CalculateMarshallingSize(cntr);
cntr.Add(slen.value);
}Personally, I like to use [font=courier new,courier,monospace]assert[/font] whenever i have to deal with a pointer.
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement