59 replies to this topic

[wood_brian]

"we need a greater appreciation of incremental (engineering) improvements with a relevance to real-world systems. “That’s just engineering, and we’re computer scientists” is an attitude we can’t afford." Bjarne Stroustrup

I agree with that and am posting here to ask for some ideas on how to improve the code here --
http://webEbenezer.n...ntegration.html . One area that needs work is settling on a coding convention and adhering to it -- the names of classes/functions/files aren't consistent. Thoughts on that are appreciated, but I hope there will be other ideas. Tia.

[e‍dd]

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 "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).

I counted a grand total of 2 asserts

[Mihai Moldovan]

/*
 * 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).

[e‍dd]

This is just wrong, you should feel ashamed.

There's no shame in doing something incorrectly.

[Cornstalks]

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, new can be made to return NULL/nullptr upon failure if you use nothrow. In which case, new may return NULL. 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 malloc instead of new.

While edd² 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.

Edited by Cornstalks, 01 May 2012 - 12:02 AM.

[Mihai Moldovan]

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.

[mhagain]

This is just wrong, you should feel ashamed.

There's no shame in doing something incorrectly.

+1. The only shame lies in not learning why it was incorrect.

[wood_brian]

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, new can be made to return NULL/nullptr upon failure if you use nothrow. In which case, new may return NULL. 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 malloc instead of new.

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² 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.

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.

I think I'll go with this one:

bl = (boolRep != 0) ? true : false;

This is a small thing, but these things add up.

Edited by wood_brian, 01 May 2012 - 12:07 AM.

[wood_brian]

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 "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).

I counted a grand total of 2 asserts

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.

[Cornstalks]

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 assert whenever i have to deal with a pointer.

[boblehest]

bl = (boolRep != 0) ? true : false;

which is slightly common

Really?
I've actually seen code like that several places, in the worst case (imo), like this:

return isEnabled ? true : false;

Which of course is equivalent to

return isEnabled;

The only different is that the first one is harder to understand, and takes longer to type.

Edited by boblehest, 01 May 2012 - 09:42 AM.

[Antheus]

bl = (boolRep != 0) ? true : false;

Idiomatic approach, one accepted in C (standard for embedded) as well as many other languages.

b1 = !!boolrep;

! obviously stands for negation.

Result of the expression is a true boolean value and it works even without explicit bool type. As a common idiom, it also behaves intuitively.

[wood_brian]

Idiomatic approach, one accepted in C (standard for embedded) as well as many other languages.

b1 = !!boolrep;

! obviously stands for negation.

Result of the expression is a true boolean value and it works even without explicit bool type. As a common idiom, it also behaves intuitively.

OK, I'll give that a shot.

Feel free to make C++ 11 recommendations.

Currently I have this:

This snippet is from cmwAmbassador.cc

#ifdef ENDIAN_BIG
		  , byteOrder(most_significant_first)
#else
		  , byteOrder(least_significant_first)
#endif

and this from cmwAmbassador.hh

#ifdef ENDIAN_BIG
  ::cmw::ReceiveBufferTCPCompressed<LeastSignificantFirst> cmwBuf;
#else
  ::cmw::ReceiveBufferTCPCompressed<SameFormat> cmwBuf;
#endif

  uint8_t const byteOrder;

I want to rewrite that as the following in the .hh file.

#ifdef ENDIAN_BIG
  ::cmw::ReceiveBufferTCPCompressed<LeastSignificantFirst> cmwBuf;
  uint8_t const byteOrder = most_significant_first;				  
#else
  ::cmw::ReceiveBufferTCPCompressed<SameFormat> cmwBuf;
  uint8_t const byteOrder = least_significant_first;
#endif

But VS 11 doesn't have support for that yet. I hope Microsoft will add support for that soon.

Edited by wood_brian, 01 May 2012 - 11:11 PM.

[e‍dd]

Perhaps you could show an example of where you would add an assert.

Everywhere you assume something. Really that's your job!

Some examples, though:

• cmwAmbassador.cc, after getaddrinfo_wrapper(), assert(res != 0)
• direct.cc, same again
• lil_string.h, strdup_never_null, assert(s != 0);
• same again in lil_string constructor
• ...

There are probably higher-level invariants that you're more aware of than I am, given you know what it's meant to be doing.

While I flick through the code again, I notice that you tend not to initialize variables that are immediately passed as references to functions that fill them in. I always think it's a good idea to initialize everything, as any place where you introduce the possibility for the behaviour of a program to differ between debug and optimized builds is asking for trouble; while debugging I'd take an impossible '0' or '-1' over an unexpected-but-entirely-plausible '273981' any day. The C++ standard gives the programmer quite enough rope in this regard already, IMO.

[wood_brian]

Everywhere you assume something. Really that's your job!

Some examples, though:

• cmwAmbassador.cc, after getaddrinfo_wrapper(), assert(res != 0)
• direct.cc, same again

Here's the code for that function

void
getaddrinfo_wrapper(char const * node
					, int port
					, addrinfo** res
				   )
{
  ::std::stringstream out;
  out << port;
  addrinfo hints;
  ::std::memset(&hints, 0, sizeof(hints));
  hints.ai_family = AF_UNSPEC;
  hints.ai_socktype = SOCK_STREAM;
  int err;
  if ((err = getaddrinfo(node
									 , out.str().c_str()
									 , &hints
									 , res
									)) != 0) {
	throw failure("Getaddrinfo: ") << gai_strerror(err);
  }
}

Something is wrong with getaddrinfo if it doesn't set res and it doesn't fail.

• While I flick through the code again, I notice that you tend not to initialize variables that are immediately passed as references to functions that fill them in. I always think it's a good idea to initialize everything, as any place where you introduce the possibility for the behaviour of a program to differ between debug and optimized builds is asking for trouble; while debugging I'd take an impossible '0' or '-1' over an unexpected-but-entirely-plausible '273981' any day. The C++ standard gives the programmer quite enough rope in this regard already, IMO.

OK.

[Washu]

malloc_never_null is flat out broken. if you ever encounter a situation where malloc does return null, it returns it for one reason: it could not allocate sufficient space.

new returns null if you pass it std::nothrow. If you do not then when it FAILS TO ALLOCATE it throws an exception. It does not infinitely loop.

Your function naming is atrocious, and leaves much to be desired. You mix case with underscores and some of the names are flat out confusing. There seems to be little divide between the public interface that you expect users to use and the private interface that you expect your internals to operate against. You are using C++, however you are not using C++ allocations (new) and using C style ones (malloc). You provide your own string implementation, which is worse than std::string in many ways and provides no inherit benefit to using it over that of just using std::string. Your use of pre-processor definitions is also terrible, with potential name collisions existing in trivial cases.

Edited by Washu, 02 May 2012 - 11:40 PM.

[wood_brian]

malloc_never_null is flat out broken. if you ever encounter a situation where malloc does return null, it returns it for one reason: it could not allocate sufficient space.

new returns null if you pass it std::nothrow. If you do not then when it FAILS TO ALLOCATE it throws an exception. It does not infinitely loop.

Your function naming is atrocious, and leaves much to be desired. You mix case with underscores and some of the names are flat out confusing.

What function names do you think are confusing?

There seems to be little divide between the public interface that you expect users to use and the private interface that you expect your internals to operate against. You are using C++, however you are not using C++ allocations (new) and using C style ones (malloc).

That's not true. The only file that uses malloc is lil_string.hh. I picked up that code on line and haven't changed it much except to make it throw an exception if the size of the string grows over 255.

You provide your own string implementation, which is worse than std::string in many ways and provides no inherit benefit to using it over that of just using std::string. Your use of pre-processor definitions is also terrible, with potential name collisions existing in trivial cases.

The purpose of the string class is limited, but if you know you don't need a string of length more than 255 then by using lil_string we can marshall the length of the string in one byte. What pre-processor definitions are you referring to?

[Washu]

malloc_never_null is flat out broken. if you ever encounter a situation where malloc does return null, it returns it for one reason: it could not allocate sufficient space.

new returns null if you pass it std::nothrow. If you do not then when it FAILS TO ALLOCATE it throws an exception. It does not infinitely loop.

Your function naming is atrocious, and leaves much to be desired. You mix case with underscores and some of the names are flat out confusing.

What function names do you think are confusing?

Have you actually LOOKED at your function naming scheme? Here's a prime example of what's wrong with your code: http://webebenezer.net/misc/MarshallingFunctions.hh

Look at that mess, it's terrible. You have functions that start with lower case letters, you have ones that start with upper case letters, you have ones that contain UNDERSCORES in their names. Pick a style and stick with it.

There seems to be little divide between the public interface that you expect users to use and the private interface that you expect your internals to operate against. You are using C++, however you are not using C++ allocations (new) and using C style ones (malloc).

That's not true. The only file that uses malloc is lil_string.hh. I picked up that code on line and haven't changed it much except to make it throw an exception if the size of the string grows over 255.

That still doesn't fix the problem then. A malloc of ONE BYTE can fail if you're out of memory. In which case we're right back at the infinite loop scenario.

You provide your own string implementation, which is worse than std::string in many ways and provides no inherit benefit to using it over that of just using std::string. Your use of pre-processor definitions is also terrible, with potential name collisions existing in trivial cases.

The purpose of the string class is limited, but if you know you don't need a string of length more than 255 then by using lil_string we can marshall the length of the string in one byte. What pre-processor definitions are you referring to?

This has, quite literally, NOTHING AT ALL to do with implementing your own string class which is worse than std::string. In fact, that's entirely an ENCODING issue when writing the data and can be avoided entirely. Here's a hint: 7 bit integer encoding. Also, your lil_string class is in the global namespace, and not in...say the cmw namespace.

As for your preprocessor definitions... the issue is with ALL OF THEM. Firstly we have WIN_DOWS... not like CMW_WINDOWS or something that makes sense, just a random WIN_DOWS. Then we have your inclusion guards... yay! lowercase. Also not included in all of your headers. Basically, your existing inclusion guards and your WIN_DOWS preprocessor definitions are trivial cases for having a collision against user code definitions or third party libraries the user may be using. This is why, for instance, all BOOST preprocessor definitions start with "BOOST_".

Edited by Washu, 03 May 2012 - 02:01 AM.

[wood_brian]

Have you actually LOOKED at your function naming scheme? Here's a prime example of what's wrong with your code: http://webebenezer.n...ingFunctions.hh

Look at that mess, it's terrible. You have functions that start with lower case letters, you have ones that start with upper case letters, you have ones that contain UNDERSCORES in their names. Pick a style and stick with it.

I'm not arguing about that. I've already said it needs work. I mentioned that in the original post. You didn't answer my question about the function names that you find confusing.

That still doesn't fix the problem then. A malloc of ONE BYTE can fail if you're out of memory. In which case we're right back at the infinite loop scenario.

Understood. People don't seem to believe that I agree there are problems with this file and I plan on fixing them.

This has, quite literally, NOTHING AT ALL to do with implementing your own string class which is worse than std::string. In fact, that's entirely an ENCODING issue when writing the data and can be avoided entirely. Here's a hint: 7 bit integer encoding. Also, your lil_string class is in the global namespace, and not in...say the cmw namespace.

I'm already using 7 bit integer encoding --
http://webEbenezer.n...ling_integer.hh
http://webEbenezer.n...ling_integer.cc
MarshallingFunctions.hh uses marshalling_integer when it marshals ::std::string and char-based strings. No one is forced to use lil_string.

As for your preprocessor definitions... the issue is with ALL OF THEM. Firstly we have WIN_DOWS... not like CMW_WINDOWS or something that makes sense, just a random WIN_DOWS. Then we have your inclusion guards... yay! lowercase. Also not included in all of your headers. Basically, your existing inclusion guards and your WIN_DOWS preprocessor definitions are trivial cases for having a collision against user code definitions or third party libraries the user may be using. This is why, for instance, all BOOST preprocessor definitions start with "BOOST_".

Good point about the prefix. I'll work on changing those macro names.

[rip-off]

You didn't answer my question about the function names that you find confusing.

Oh, I need to frobnicate that foo. Should I write:

• frobnicateFoo?
• FrobnicateFoo?
• frobnicate_foo?
• Frobnicate_Foo?
• ...

This is intellectual overhead that benefits no one. In most libraries, if you can see at least one other function call you know the format of all of them. Not so if the naming convention is inconsistent.

I'm already using 7 bit integer encoding

Great! Unfortunately, this nullifies the only benefit of lil_string you've given:

... if you know you don't need a string of length more than 255 then by using lil_string we can marshall the length of the string in one byte.