Thread: Code review

Originally Posted by phantomotap

Such as? Such as?! You said that code compiled! You said it yielded only a linker error! That alone says that the MSVC2k8 compiler isn't doing its job. The MSVC2k3 compiler, and presumably any advanced release, is a good compiler; it has good conformance, but it will let many illegal constructs pass because the are what it historically supports. To learn real C++ you need a compiler that will complain about everything. Actually, you need several compilers that will complain about everything that way you don't wind up coding for a preference.

Ah... it's a shame. Looking at the differences would go a lot faster.

That's why I was asking for alternative compilers.

I hope you have a differencing program installed.

Nope. That I don't.

Emacs is a nice differencing program (and it does exist for windows). Also 'Winmerge' is a nice one, easier to use if you're used to windows, and for free.

ps: Microsoft Visual Studio 2005 does have a compare option, if I'm not wrong? Look under 'edit' or so.

That's why I was asking for alternative compilers.

Borland, gnu g++, SunStudio CC, and probably many many others.

(The lack of parity is simply nonsense.)

What do you mean by this?

There will be more operators for sure. But I don't want to spend all time implementing a lot of things without needing to use them first. Too time consuming.
I'm going to give Code::Blocks + gcc another try.

There will be more operators for sure. But I don't want to spend all time implementing a lot of things without needing to use them first.

That's the advantage of implementing a minimal but complete interface instead of a monolith like what you have now

Monolith or not, I'd have to write the free functions anyway, so it would still be a "monolith."

Monolith or not, I'd have to write the free functions anyway, so it would still be a "monolith."

No, you could write the free functions later, when you decide you would like or need to have them.

And I can extend the class interface later, too. No difference there.
It may be a difference if I don't have access to the source or multiple individuals are working on it, but if it's just me, I gain not one single benefit.

And I can extend the class interface later, too. No difference there.

Since your class is not a polymorphic base class, the only way to extend the class interface other than by the use of non-member non-friend functions would be to change the class itself. I daresay that this is a violation of the open-closed principle, even though that principle has abstraction, rather than minimal and complete interfaces, in mind.

It may be a difference if I don't have access to the source or multiple individuals are working on it, but if it's just me, I not one single benefit.

On the other hand, if you are the designer, sole implementer, and sole user of the class, the benefit of extension without modifying the class itself is reduced, since you can change the interface at a whim.

Still, I would argue that if you make a clear distinction between what is necessary and what is convenient, it would help you prioritise so that you do not "spend all time implementing a lot of things without needing to use them first".

Originally Posted by laserlight

Since your class is not a polymorphic base class, the only way to extend the class interface other than by the use of non-member non-friend functions would be to change the class itself. I daresay that this is a violation of the open-closed principle, even though that principle has abstraction, rather than minimal and complete interfaces, in mind.

Making it expendable via polymorphism and inheritance is not ruled out. But first I will make sure I finish the class interface (ie, make it stable).

Still, I would argue that if you make a clear distinction between what is necessary and what is convenient, it would help you prioritise so that you do not "spend all time implementing a lot of things without needing to use them first".

I agree 100%. Right now, it contains mostly all the functionality I typically need.
If it means I need something later, I will simply add it to the interface. The point of the class was to merge all the free utility functions to the class.
This isn't the only class I'm planning on. I'm planning on abstracting away a lot of things, especially with Win32 API.


I ran the code through GCC and fixed a lot of errors, but I still can't figure out what needs to be made to fix the linking errors.
The code should be (or almost?) identical to the given solution, but it still won't work...

I'm attaching the latest rev.

Originally Posted by cpjust

Re: compile errors on VC++ vs. gcc
Are you compiling with Warning Level 4? VC++ defaults to Warning Level 3 for some stupid reason. You can also add /Wall to the compile options for a ton of errors.

Yes, I always use level 4 warnings.
I also ticked every option for warnings I could find in GCC. Both pedantic and -wall.

Re: friend functions
Why would your == operators need to be friends? As long as they don't try to use any private data or functions, they can be non-member AND non-friend functions.

Well... I would say yes, seeing as it must have access to internals to assign.
But even if not, I'd like to understand how it works.

Re: Diff program
I believe the non-express VC++ should come with WinDiff, but I prefer the Diff program in Perforce. You should install Perforce anyways, if you don't already have a version control system. http://www.perforce.com/
It's free to use for single-user usage, and it's much more intuitive and easier to use than something like CVS or even VSS.

Hmmm. I don't see WinDiff.
I'll check out Perforce.


Ah, it compiles now.
I'm guessing the <> tags plus explicit template tags are required to compile it correctly. That plus you need declarations of the template names that you reference with the templates:

Code:

friend CTmplStringBase<T, Traits>& operator += <> (CTmplStringBase<T, Traits>& rlhs, const T* strData);

Green = Explicit type for the class
Red = Needed to mention it's a template I guess.

Code:

	template<typename T, typename Traits> class CTmplStringBase;

Needed to tell the compiler the template names mentioned above really do exist.

Originally Posted by Elysia

You did remind me that I don't have an index operator either...
I'm not that familiar with iterators either, but it will probably increase operability with STL, so yes, I'm probably going to take a look at it.

Since you have a cast operator to a pointer, you can already use [] as it will just do this on the pointer. If you also implement yourself then you'll probably find it's ambiguous.

In other words, implementation such as strcmp - return if the string is "lower", equal or "higher" than compared string?

Yes your operator < can quite happily return strcmp(a.p, b.p) < 0 for example. As long as the operator is provided then using the class in a set is a snap.

That's unfamiliar to me. Perhaps you could elaborate a little?

As you perhaps know, there's this template function called std::swap. It does the whole temp T = a; a = b; b = temp; thing. Now what that means is that for your class it calls the assignment operator 3 times to swap two instances of the class, requiring 3 allocations and 3 deallocations within your class. However you can provide a specialisation for std::swap that takes your string class and instead of calling the assignment operator it can simply swap the pointers and sizes. That way no memory allocation/deallocation is required. Then when swap is called by things like std::sort, those things will operate faster. Of course some of us also use std::swap at times in our own code for efficiency also.

m_nSize keeps track of the full size of character buffer in characters.
m_pData is a pointer to the start of the buffer.
m_pUnusedStart is a pointer to where the "unused" portion of the buffer begins or in other words, where it should append data.

Oh okay, so m_nSize is not the string length, I get you now.

After taking a look at your StringExImpl.h file, there are more things I would like to advise you on:

In the destructor, don't bother to set m_nSize to zero. Integers don't require destruction - it's excess baggage.

Your copy-constructor is very contrived and inefficnent. First you call Init which allocates the default sized string of 10 chars. That's already a bad sign as there's a pretty good chance that it wont fit in that, and you'll have to destroy it straight afterwards. That can unnecessarily add to heap fragmentation.
Then you call the assignment operator. The assignment operator calls CopyStr. CopyStr calls PrepareBuffer and then CopyStrLowLevel.
Starting from that call to PrepareBuffer is where it gets really nasty. Why would the nNeededSize you pass in ever care about the current length? Why is the new size allocated the length of the right hand string*2 plus 2 plus the default size*2? That's pretty excessive!
Then it calls CopyStrLowLevel to copy the old empty string to the new buffer. Then you do the unthinkable and call the destructor directly. Sure you're allowed to do that, but Ewwww! The only useful thing the destructor does is delete [] m_pData so why not just do that? Then finally it copies the old string across into the new oversized buffer. It's all far too much code being used just to do copy construction which is a very common operation.

Here's my attempt at writing your copy constructor, that does exactly that, produces an exact copy of the original. Even the unused buffer space is the same in the copy, as it should be IMHO.

Code:

	template<typename T> CTmplStringBase<T>::CTmplStringBase(const CTmplStringBase<T>& rSrc)
	: m_pData(new T[rSrc.m_nSize]), m_nSize(rSrc.m_nSize)
	{
		m_pUnusedStart = m_pData + rSrc.GetLength();
		uint32_t nSize = rSrc.GetLength() + 1;
		memcpy_s(m_pData, nSize * sizeof(T), rSrc.m_pData, nSize * sizeof(T));
	}

You seem to use va_start without va_end. I'm pretty sure that's a bad thing.

I can't work out why Assign would call Empty then operator +=.

It does not make sense for the 1 parameter version of GetLength, GetFormatFunction, GetVFormatFunction, GetVFormatCountFunction, and GetCompareFunction to be member template functions. You've totally mixed up how to do that. What you want is for example with GetCompareFunction, a default template that provides no implementation. Then you provide specialisations with the template<> syntax where instead of using the type in any way at all you use char and wchar_t.
http://www.cprogramming.com/tutorial...alization.html
http://www.cplusplus.com/doc/tutorial/templates.html

I'm glad you've done the two-pass approach for your Replace function.