7/26/2010

07-26-10 - Code Issues

How do I make a .c/.cpp file that's optional? eg. if you don't build it into your project, then you just don't get the functionality in it, but if you do, then it magically turns itself on and gives you more goodies.

I'll give you a particular example to be concrete, though this is something I often want to do. In the RAD LZH stuff I have various compressors. One is a very complex optimal parser. I want to put that in a separate file. People should be able to just include rrLZH.cpp and it will build and run fine, but the optimal parser will not be available. If they build in rrLZHOptimal, it should automatically provide that option.

I know how to do this in C++. First rrLZH has a function pointer to the rrLZHOptimal which is statically initialized to NULL. The rrLZHOptimal has a CINIT class which registers itself and sets that function pointer to the actual implementation.

This works just fine (it's a standard C++ self-registration paradigm), but it has a few problems in practice :

1. You can run into order-of-initialization issues if you aren't careful. (this is not a problem if you are a good C++ programmer and embrace proper best practices; in that case you will be initializing everything with singletons and so on).

2. It's not portable because of the damn linkers that don't recognize CINIT as a binding function call, so the module can get dropped if it's in a lib or whatever. (this is the main problem ; it would have been nice if C++0x had defined a way to mark CINIT constructions as being required in the link (not by default, but with a __noremove__ modifier or something)). There are various tricks to address this but I don't think any of them is very nice. (*)

I general I like this pattern a lot. The more portable version of this is to have an Install() function that you have to manually call. I *HATE* the Install function pattern. It causes a lot of friction to making a new project because you have to remember to call all the right Installs, and you get a lot of mysterious failures where some function just doesn't work and you have to go back and install the right thing, and you have to install them in the right order (C++ singleton installs mostly take care of order for you). etc.

(* : this is one of those issues that's very annoying to deal with as a library provider vs. an end-application developer. As an app developer you just decide "this is how we're doing it for our product" and you have a method that works for you. As a library developer you have to worry about people not wanting to use the method you have found that works, and how things might behave under various compilers and usage patterns. It sucks.)

ADDENDUM : the problem with the manually-calling Install() pattern is that it puts the selection of features in the wrong & redundant place. There is one place that I want to select my modules, and that is in my build - eg. which files I compile & link, not in the code. The problem with it being in the code is that I can't create shared & generic startup code that just works. I wind up having to duplicate startup code to every app, which is very very bad for maintainability. And of course you can't make a shared "Startup()" function because that would force you to link in every module you might want to use, which is the thing you want to avoid.


For the PS3 people : what would be the ideal way for me expose bits of code that can be run on the SPU? I'm just not sure what people are actually using and how they would like things to be presented to them. eg. should I provide a PPU function that does all the SPU dispatching for you and do all my own SPU management? Is it better if I go through SPURS or some such? Or should I just provide code that builds for SPU and let you do your management?


I've been running into a problem with the MSVC compiler recently where it is incorrectly merging functions that aren't actually the same. The repro looks like this. In some header file I have a function sort of like this :

StupidFunction.h :

inline int StupidFunction()
{
  return SOME_POUND_DEFINE;
}

Then in two different files I have :
A.cpp :

#define SOME_POUND_DEFINE  (0)
#include "StupidFunction.h"

printf("%d\n",StupidFunction());

and :
B.cpp :

#define SOME_POUND_DEFINE  (1)
#include "StupidFunction.h"

printf("%d\n",StupidFunction());

and what I get is that both printfs print the same thing (random whether its 0 or 1 depending on build order).

If I put "static" on StupidFunction() it fixes this and does the right thing. I have no idea what the standard says about compilation units and inlines and merging and so on, so for all I know their behavior might be correct, but it's damn annoying. It appears that the exact definition of inline changed in C99, and in fact .cpp and .c have very different rules about inlines (apparently you can extern an inline which is pretty fucked up). (BTW the whole thing with C99 creating different rules that apply to .c vs .cpp is pretty annoying).

ADDENDUM : see comments + slacker.org advice about inline best practice (WTF, ridiculous) , and example of GCC inline rules insanity


In other random code news, I recently learned that the C preprocessor (CPP) is not what I thought.

I always thought of CPP as just a text substitution parser. Apparently that used to be the case (and still is the case for many compilers, such as Comeau and MSVC). But at some point some new standard was introduced that makes the CPP more tightly integrated with the C language. And of course those standards-nazis at GCC now support the standard.

The best link that summarizes it IMO is the GCC note on CPP Traditional Mode that describes the difference between the old and new GCC CPP behavior. Old CPP was just text-sub, New CPP is tied to C syntax, in particular it does tokenization and is allowed to pass that tokenization directly to the compiler (which does not need to retokenize).

I guess the point of this is to save some time in the compile, but IMO it's annoying. It means that abuse of the CPP for random text-sub tasks might not work anymore (that's why they have "traditional mode", to support that use). It also means you can't do some of the more creative string munging things in the CPP that I enjoy.

In particular, in every CPP except GCC, this works :


#define V(x) x
#define CAT(a,b)  V(a)V(b)

to concatenate two strings. Note that those strings can be *anything* , unlike the "##" operator which under GCC has very specific annoying behavior in that it must take a valid token on each side and produce a valid token as output (one and only one!).


In further "GCC strictness is annoying", it's fucking annoying that they enforce the rule that only ints can be constants. For example, lots of code bases have something like "offsetof" :


/* Offset of member MEMBER in a struct of type TYPE. */
#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)

well, that's illegal under GCC for no damn good reason at all. So you have to do :

/* Offset of member MEMBER in a struct of type TYPE. */
#ifndef __GNUC__
#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
#else
/* The cast to "char &" below avoids problems with user-defined
   "operator &", which can appear in a POD type.  */
#define offsetof(TYPE, MEMBER)                                  \
  (__offsetof__ (reinterpret_cast <size_t>                      \
                 (&reinterpret_cast <const volatile char &>     \
                  (static_cast<TYPE *> (0)->MEMBER))))
#endif /* C++ */

damn annoying. (code stolen from here ). The problem with this code under GCC is that a "type *" cannot be used in a constant expression.

A similar problem comes up in templates. On every other compiler, a const pointer can be used as a template value argument, because it's just the same as an int. Not on GCC! In fact because they actually implement the standard, there's a new standard for C++0x which is going to make NULL okay, but only NULL which is also annoying because there are places I would use arbitrary values. (see for example 1 or 2 ).

ADDENDUM : a concrete example where I need this is my in-place hash table template. It's something like :


template < typename t_key,t_key empty_val,t_key deleted_val >
class hash_table
{

that is, I hash keys of type t_key and I need a value for "empty" and "deleted" special keys for the client to set. This works great (and BTW is much faster than the STL style of hash_map for many usage patterns), but on GCC it doesn't work if t_key is "char *" because you can't template const pointer values. My work-around for GCC is to take those template args as ints and cast them to t_key type internally, but that fucking blows.

In general I like to use template args as a way to make the compiler generate different functions for various constant values. It's a much cleaner way than the #define / #include method that I used above in the static/inline problem example.

14 comments:

Jeff Roberts said...

> Or should I just provide
> code that builds for SPU
> and let you do your
> management?

We have good stuff for this already (that *everyone* uses - even the hardcore guys like Insomniac and Naughtydog).

I'll get you the sweet action tonight.

->Jeff

IvyMike said...

I know it is not much consolation, but I think the inline behavior falls into (as you guessed) the "correct, but [...] damn annoying" bin.

I'd like to cite Stroustroup, just in case my memory is faulty, but don't have it handy. But... the IBM C++ compiler has almost exactly the example you have above, and describes that the C++ standard makes it so.

http://publib.boulder.ibm.com/infocenter/compbgpl/v9v111/topic/com.ibm.xlcpp9.bg.doc/language_ref/cplr243.htm#cplr243

Carsten Orthbandt said...

The issue with the two functions being merged into one is simply explained by the "one definition rule".
You end up with two functions which both have external linkage and the exact same signature. When asked to resolve the calls, the linker uses the first one it finds.
By marking them as "static" you avoid the external linkage, making them elements that are only visible and linked within that CPP file.

cbloom said...

"You end up with two functions which both have external linkage and the exact same signature."

Well yeah, if you think they have external linkage the problem is obvious.

IMO "inline" should imply "static". I have no interest in extern inlines.

cbloom said...

And in fact that IBM Compiler that is so aware of the problem has a "-qstaticinline" option to make all inlines static.


Annoyingly I can't just put "static" on my #define INLINE to fix it all over my code, because I sometimes put INLINE on member functions (when they have their body in a header but inside the class definition), and "static" on member functions doesn't mean the same thing.

  said...
This comment has been removed by the author.
ryg said...

"In further "GCC strictness is annoying", [..]"
Actually, C++ on GCC is annoying, period.

It's got a lot of good stuff and then some totally ridiculous warnings you can't turn off (unless you set the warning level extremely low, which is also retarded). Not sure what the exact message is, but one that's particularly annoying is "comparison is always true for this data type" when you do something like "if (x >= 0)" where x is unsigned. Easy to get when you have a template that's instantiated using both signed and unsigned types. We had something like this in a printf-like function that was templated on character type and existed in variants for ASCII (7-bit only, just for debugging) and UTF-16 (used for all visible / localized text). There was one check in there that tested "if (some_char < 128)", which is always true for chars if "char" is signed, but not always true for the Unicode variant. So we got that warning on every file that instantiated the template for char. Way to go. I think they finally added a switch to disable that warning in GCC 4.4 - I looked it up, the corresponding bug was filed something like 7 years ago.

Another one: Where it complains when elements in an initializer list are in a different order than in the class declaration. Now that's actually a good idea if there's ctor calls in there, but complaining about it when you're just initializing POD values is retarded, and this is the kind of stuff that gets unintentionally broken all the time in cross-platform projects because no other compiler complains about it.

cbloom said...

"Not sure what the exact message is, but one that's particularly annoying is "comparison is always true for this data type" when you do something like "if (x >= 0)" where x is unsigned. "

Yep, this is one of the annoying ones I've been hitting.

This is -Wtype-limits , but oddly there is no -Wno-type-limits (most GCC warnings have a -Wno variant to turn them off).

I think the only way to solve this is to start with all warnings off and then turn on just the ones you want one by one. :(

"Another one: Where it complains when elements in an initializer list are in a different order than in the class declaration. Now that's actually a good idea if there's ctor calls in there, but complaining about it when you're just initializing POD values is retarded"

Yep this one is super annoying and I have the exact same complaint as you.

If they were actually complex ctor calls that could somehow affect each other, I would love to know about this warning, though it would be very badly written code if they actually were order-dependent.

Anonymous said...

"1. You can run into order-of-initialization issues if you aren't careful. (this is not a problem if you are a good C++ programmer and embrace proper best practices; in that case you will be initializing everything with singletons and so on)."
Do not know what the rest of the article is saying as I stopped reading after the above note. Best practices != Singleton!

RCL said...

"Another one: Where it complains when elements in an initializer list are in a different order than in the class declaration. Now that's actually a good idea if there's ctor calls in there, but complaining about it when you're just initializing POD values is retarded"

It's actually not retarded even if you initialize POD values, because you may use function calls to calculate initial values for them and it gives you an extra warning that the order will be different from what you thought.

GCC is sometimes pedantic but as long as it doesn't prevent the compiler from generating optimal code, I'm happy to adhere to stricter practices (after all, why encourage sloppy coding?)

cbloom said...

class C { int x,y; C() : y(1), x(2) { } }

that's not sloppy coding and there's no damn reason to warn me about it.

I don't think I have ever in my life put a (non trivial) function call in the initializer list.

I know that strict C++ people like to use the initializer list because you avoid double-setting values and its better for exceptions and so on, but for debuggability and readability I prefer to put any kind of complex intialization in the function body.

cbloom said...

BTW this all wouldn't be so bad if GCC would just give me a damn warning ID when it spits out a warning !!!!

At least with MSVC they put the warning # right there, so when they give me a bogus warning I can just go disable it.

With GCC I have to go to fucking Google and try to figure out what warning that is and if it's even *possible* to disable it.

And then I can't disable it in the code file just where I want to.

GRR GRR GRR

If I take off my whiney hat, it's actually amazing how good it is, given that it's pretty democratic open source. As usual with open source stuff, lots of people want to work on the sexy bits (the scheduler, etc), and nobody wants to work on the usability.

RCL said...

I believe that token-based approach is more preferable, because maintaining a database of IDs for all GCC versions and platforms would be much harder than it is for MSVC.

Spot on about free software. As someone said, "People draw for free. People don't clean toilets for free."

Jonathan said...

gcc does have -fdiagnostics-show-option, which will indicate a flag that can be used to control a warning (if there is one). IIRC, gcc-4.5 has some improvements related to warnings.

In the little testing I've done with it, 4.5 has generated significantly better performing output than 4.4 for some SPU programs I tested (one fairly simple case being ~30% faster).

old rants