Code review

This is a discussion on Code review within the C++ Programming forums, part of the General Programming Boards category; Do enlighten me. Even if just a bit. Every one bit helps. Honestly? I'm fairly certain most of the mechanical ...

  1. #46
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    4,213
    Do enlighten me. Even if just a bit. Every one bit helps.
    Honestly? I'm fairly certain most of the mechanical problems are just mistakes you'll soon catch, but the design problems are something that you'll have to lose your prejudice of OO concepts to correct. (The lack of parity is simply nonsense.) Some of the other problems relate to C++ mechanics. (Unless I'm very much mistake, a complex assignment statement regarding self will garbage memory.) Some are probably just a lack of experience with advanced template/meta-programming. (Your traits class simply isn't doing its job.)

    Such as? Visual Studio compiler has always been helpful with warnings.
    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.

    Nope. That I don't.
    Ah... it's a shame. Looking at the differences would go a lot faster.

    Soma

  2. #47
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,537
    Quote Originally Posted by phantomotap View Post
    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.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  3. #48
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    4,213
    That's why I was asking for alternative compilers.
    Ah. Forgive me. I read that as: and what problems would these be that it didn't mention.

    I have about two dozen installed on all of my machines--some running through a VM. If you were on an SGI you'd probably be set. For "Wintel" machines, an installation of GCC 4.2 would have caught at least some of the problems I noticed. Now, it wouldn't have told you what to do, but it would have given you focus. You should also get any good EDG backed compiler. (Either the Intel C++ compiler from 9.0 or the Comeau C++ compiler from 4.0 I think should do nicely.)

    Or, I think, PC-Lint should work as well in the general case, but it probably wouldn't have caught the friend template function problem.

    Soma

  4. #49
    Registered User
    Join Date
    May 2008
    Location
    Paris
    Posts
    248
    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?
    Last edited by MarkZWEERS; 05-10-2008 at 11:52 AM. Reason: added comment on comparing

  5. #50
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    4,213
    Borland, gnu g++, SunStudio CC, and probably many many others.
    Not really... the last Sun CC I used is virtually useless for this and Borland has fallen quite far--playing nicely with Microsoft I fear. (Either may have options I didn't know about at the time.)

    (The lack of parity is simply nonsense.)
    The lack of ~20 comparison/conversion operators.

    Soma

  6. #51
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,537
    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.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  7. #52
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,588
    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
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  8. #53
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,537
    Monolith or not, I'd have to write the free functions anyway, so it would still be a "monolith."
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  9. #54
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,588
    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.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  10. #55
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,537
    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.
    Last edited by Elysia; 05-10-2008 at 12:35 PM.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  11. #56
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,588
    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".
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  12. #57
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,537
    Quote Originally Posted by laserlight View Post
    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.
    Attached Files Attached Files
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  13. #58
    and the hat of sweating
    Join Date
    Aug 2007
    Location
    Toronto, ON
    Posts
    3,545
    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.

    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.

    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.

  14. #59
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,537
    Quote Originally Posted by cpjust View Post
    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.
    Last edited by Elysia; 05-10-2008 at 03:10 PM.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  15. #60
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,299
    Quote Originally Posted by Elysia View Post
    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.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

Page 4 of 5 FirstFirst 12345 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Code Review for upcoming contest
    By Darryl in forum Contests Board
    Replies: 2
    Last Post: 02-28-2006, 01:39 PM
  2. Problem : Threads WILL NOT DIE!!
    By hanhao in forum C++ Programming
    Replies: 2
    Last Post: 04-16-2004, 01:37 PM
  3. True ASM vs. Fake ASM ????
    By DavidP in forum A Brief History of Cprogramming.com
    Replies: 7
    Last Post: 04-02-2003, 03:28 AM
  4. Interface Question
    By smog890 in forum C Programming
    Replies: 11
    Last Post: 06-03-2002, 05:06 PM
  5. Replies: 0
    Last Post: 02-21-2002, 05:05 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21