Critique of code quality
in [C++]
|
Prev: How to reuse an existing stream with a different streambuf ?
Next: A quick question about Initializer-List in C++0x
From: Ulrich Eckhardt on 10 Jun 2010 20:34 Peter Olcott wrote: > On 6/9/2010 2:06 PM, Nick Hounsome wrote: >> On 9 June, 18:06, Peter Olcott<NoS...(a)OCR4Screen.com> wrote: >>> On 6/9/2010 8:25 AM, Nick Hounsome wrote: >>>> c) Why do you want it [a library] in a header? >>> >>> Two files is redundant and messy. All of my code is in headers. >> >> Without going into details either thousands of C++ programmers are >> doing it wrong and you have had a blinding insight that nobody else >> has thought of before or just maybe they are right. > > Or my way works better for small projects where the additional > compile-time is moot. I make essentially all of my choices based on > reasons rather than rules. For small projects where I am the sole > developer, this practice works better for me. Actually, you are not alone. I have a bunch of smallish libraries that are just added to a project using #include. That said, some of these are still separated into header/implementation, nobody says you can't #include an implementation file. Of course, you have to do that exactly once, but how difficult is that? Uli -- Sator Laser GmbH Geschäftsführer: Thorsten Föcking, Amtsgericht Hamburg HR B62 932 [ See http://www.gotw.ca/resources/clcm.htm for info about ] [ comp.lang.c++.moderated. First time posters: Do this! ]
From: Nick Hounsome on 11 Jun 2010 07:34 On 10 June, 21:30, Peter Olcott <NoS...(a)OCR4Screen.com> wrote: > On 6/9/2010 2:06 PM, Nick Hounsome wrote: > >>>>> 3) Eliminate all the global stuff - It's not comp.lang.c here. > > >>>> I don't know what you mean, everything is a member of a class. > > >>> all the const uint8_t have global scope > > >> OK what is the best way to do this? > > > Move them into the class (as enums) > > I might get a compile-time warning about assigning a (32-bit?) enum to > an 8-bit unsigned char. Why? You should be assigning an enum to an enum. Your states should be enums and your state table should be a table of states. > >> MS VS always inlines when you "suggest" it to. > > > Are you sure? > > Have you dissassembled this? > Yes. > > That's another black mark for MS then. > > Why? Getting more of exactly what you ask for is a good thing, as long > as you know what to ask for. But in general you don't know what to ask for. Do you know exactly what code the compiler will produce for your method? The compiler does. Therefore it is better placed to decide whether or not to inline something. Back in the day when compilers put everything that you marked as "register" in a register it was found that for most programs this made the code drastically WORSE. inline isn't quite as prone to such problems but the same principles apply. -- [ See http://www.gotw.ca/resources/clcm.htm for info about ] [ comp.lang.c++.moderated. First time posters: Do this! ]
From: Peter Olcott on 11 Jun 2010 07:43 On 6/10/2010 8:14 PM, Dilip wrote: > On Jun 9, 12:06 pm, Peter Olcott<NoS...(a)OCR4Screen.com> wrote: >> On 6/9/2010 8:25 AM, Nick Hounsome wrote: >>>>> 5) It is highly unlikely that these methods would be inlined by the >>>>> compiler (inline is a hint not an order) and if they were it could be >>>>> a space wasting disaster. Make them non-inline - The call overhead >>>>> will be dwarfed by the processing in almost all cases. >> >>>> The compiler requires inlining if the methods are included in the >>>> header. I hate creating the typically required pair of files it seems so >>>> redundant. >> >>> The compiler is never REQUIRED to inline anything in fact the keyword >>> "inline" will almost certainly go the way of "register" and be >>> deprecated some time in the future. >> >> MS VS always inlines when you "suggest" it to. > > Could you please read the documentation before making dubious > statements like this? I looked up the reference for you on MSDN. > Please read this link: > http://msdn.microsoft.com/en-us/library/z8y1yy88.aspx > (This document is specific to the VC++ compiler that ships with VS > 2010 but every other compiler version I checked carries the same > verbiage) > > The relevant statement is: > "The compiler treats the inline expansion options and keywords as > suggestions. There is no guarantee that functions will be inlined. You > cannot force the compiler to inline a particular function, even with > the __forceinline keyword. " > > I was referring to my experience with examining the generated assembly language. In other words not what the docs say that the compiler does, but, what the compiler actually does in practice. -- [ See http://www.gotw.ca/resources/clcm.htm for info about ] [ comp.lang.c++.moderated. First time posters: Do this! ]
From: Peter Olcott on 11 Jun 2010 15:12 On 6/11/2010 5:34 PM, Nick Hounsome wrote: > On 10 June, 21:30, Peter Olcott<NoS...(a)OCR4Screen.com> wrote: >> On 6/9/2010 2:06 PM, Nick Hounsome wrote: > >>>>>>> 3) Eliminate all the global stuff - It's not comp.lang.c here. >> >>>>>> I don't know what you mean, everything is a member of a class. >> >>>>> all the const uint8_t have global scope >> >>>> OK what is the best way to do this? >> >>> Move them into the class (as enums) >> >> I might get a compile-time warning about assigning a (32-bit?) enum to >> an 8-bit unsigned char. > > Why? You should be assigning an enum to an enum. Your states should be > enums and your state table should be a table of states. That would make the table 400% larger, and thus shift my priorities of readability and efficiency too far in the wrong direction without a sufficient corresponding benefit. > >>>> MS VS always inlines when you "suggest" it to. >> >>> Are you sure? >>> Have you dissassembled this? >> Yes. >>> That's another black mark for MS then. >> >> Why? Getting more of exactly what you ask for is a good thing, as long >> as you know what to ask for. > > But in general you don't know what to ask for. > Do you know exactly what code the compiler will produce for your > method? > The compiler does. Therefore it is better placed to decide whether or > not to inline something. Inlining is always better for functions that are known to only need to be called from a single place. > Back in the day when compilers put everything that you marked as > "register" in a register it was found that for most programs this made > the code drastically WORSE. inline isn't quite as prone to such > problems but the same principles apply. > > I could see how that would drastically limit the compiler's optimizations. I don't see corresponding reasoning for inline. -- [ See http://www.gotw.ca/resources/clcm.htm for info about ] [ comp.lang.c++.moderated. First time posters: Do this! ]
From: Nick Hounsome on 12 Jun 2010 00:23
On 12 June, 07:12, Peter Olcott <NoS...(a)OCR4Screen.com> wrote: > On 6/11/2010 5:34 PM, Nick Hounsome wrote: > > > >> On 10 June, 21:30, Peter Olcott<NoS...(a)OCR4Screen.com> wrote: >>> On 6/9/2010 2:06 PM, Nick Hounsome wrote: > >>>>>>>> 3) Eliminate all the global stuff - It's not comp.lang.c here. > >>>>>>> I don't know what you mean, everything is a member of a class. > >>>>>> all the const uint8_t have global scope > >>>>> OK what is the best way to do this? > >>>> Move them into the class (as enums) > >>> I might get a compile-time warning about assigning a (32-bit?) enum to >>> an 8-bit unsigned char. > >> Why? You should be assigning an enum to an enum. Your states should be >> enums and your state table should be a table of states. > > That would make the table 400% larger, and thus shift my priorities of > readability and efficiency too far in the wrong direction without a > sufficient corresponding benefit. I don't think that the standard requires enums to be stored as int it just says that they are always promoted to at least int size when used in an arithmetic context (which they wont be if you compare enum with enum rather than with uint8_t) so you're table wouldn't be any bigger. The new std allows you to forward declare opaque enums by explicitly stating the underlying type (which can be smaller than int) so you can even write stuff like: enum State : uint8_t; // State WILL be 1 byte and if any enum value defined for it wont fit it in uint8_t is a compile time error struct X { State state; }; // sizeof(X) == 1 Even if it WERE 300% bigger (400% larger = 5 times the size) it would make a one off small table that is not on the stack and not on the heap into a slightly less small table. I've done a lot of embedded work,some of it on very restricted machines and I wouldn't think twice about it in any of them. It's just too small to start thinking of premature optimization. First write good quality code. Only make it ugly when you have tried the good stuff and proved that it's too big or too slow. -- [ See http://www.gotw.ca/resources/clcm.htm for info about ] [ comp.lang.c++.moderated. First time posters: Do this! ] |