New Linux, New GCC, New Warnings

I have just upgraded to SuSe 10.3 which has gcc 4.2.1 installed and now i get a whole bunch of warnings when i compile my smaug code.



All the warnings are the same and on that type of expression. I know these are nothing to worry about, but is there any way that i can re-write those lines to not trip the warning. I like not having any warnings when i compile.

Thanks for helping.

Chances are that 'arg' is a local variable allocated on the stack, e.g.

char arg[1234];

So, it cannot be null and therefore will always have an address causing it to always be 'true'. To remove the warning, it suffices to remove the redundant check:

Is there any particular reason why the original coders wrote it in such a manner, differences in how gcc2 and gcc4 operate perhaps?

It could be many things. I see three likely scenarios:

1. It used to be checking an argument that actually could be null, such as the 'argument' parameter. But then it got changed to 'arg', and nobody bothered changing the condition.

2. Somebody typed it out of habit, because you almost always check strings for null before dereferencing them.

3. Somebody typed it because they copy/pasted and didn't know any better.


I think option 1 is most likely though. It's also probable that earlier versions of gcc didn't notice it and so didn't give a warning. (But a local array like that always has a non-null address, no matter what version of gcc you're using.)

Got another question for you David LOL,


Changing \0 to NULL removes the warning, whats the difference between NULL and \0.

Is it that NULL is a value meaning 0 and \0 is a memory location,

Well first off this is the same warning, basically; arg1 being a local buffer will never have a zero value.

As for why '\0' causes a warning but NULL doesn't, err, I don't know. :) Some systems define NULL as a special keyword for the compiler; others define it as 0; others yet define it as ((void*)0) (this last one is the most common for C I think; I read somewhere that C++ defines it as 0). Maybe the compiler can detect a comparison to zero directly, but isn't smart enough to detect comparison to the other things?

What if you try just arg1 == 0, and then arg1 == ((void*)0)? Maybe we can figure out which one it's not smart enough to figure out for the warning. :-)

This one it likes, but == 0 gives the warning.

Also, these 2 are both liked by the compiler.

Also,

if( !arg1 || !arg2 ) this gives the warning, and im going to assume that this is a fair replacement for this line,

if( arg1[0] != '\0' || arg2[0] != '\0' )

On another note, i just found i have a NULLSTR macro used locally in the liquids code, think i might have to fix it up to not give the error, make it global and use it wherever those checks are made.

That's kind of funny -- it's smart enough to realize that == 0 is comparing to zero, but == ((void*)0) isn't. :-) Ah, well...


Yeah, that's ok though, since arg1[0] is of type char and could be zero. 0 and '\0' are the same thing (== 0).


Yes that looks like a reasonable replacement, except for one thing: the original code says:

if arg1 is null (i.e. empty) or arg2 is null (i.e. empty)

The new code says:

if arg1 is NOT empty or arg2 is NOT empty

So if you change the != to == you'll be preserving the intent of the original code.

This line,


Gives this warning,
update.c:1496: warning: format not a string literal, argument types not checked

And this is how i fixed it,


There are a few other places where this happens, is there any real need to have the %s, does it serve any real purpose if only to check that your putting the right things into the variables.

Heh, you replied just a few seconds after I did. :-)

What you did doesn't quite do the same thing. The original code uses the variable string as the actual format string, and passes in the argument as a parameter to the format string. In the new version, you're just concatenating the two.

The warning is ok, though, as long as you're always sure that the passed in argument corresponds to whatever the format string is expecting. For instance if the format string has a %s in it and you pass in a number, you'll be in for trouble...

Ok i think i get what your saying. In that example

corpse_descs[UMIN( timerfrac - 1, 4 )] = abcd
bufptr = efgh

buf then = abcdefgh

where as if i had the %s %s buf would then = abcd efgh, which could screw up the formatting of some strings through out the game if i was to change those warnings to use the %s.

Actually here's a better example...



Now, buf will contain "abc".

But here:



buf contains "%s abc". The reason is that the first %s takes the value of fmtstr (which is just "%s") and the second %s takes the value of bufptr (which is "abc").

In other words by pushing the format string into the printf parameters, you make it a string just like any other. But, if you use a variable as the format string, the compiler can no longer verify at compile time that the arguments you're using make sense, which is why it's giving you a warning.

Arhhh now it makes sence. Im starting to enjoy this lesson, now onto the tough ones, const char * casts and all those scary things.

board.c:908: warning: cast discards qualifiers from pointer target type

void make_note( const char *board_name, const char *sender, const char *to, const char *subject, const int expire_days,
const char *text )

line 908>> note->sender = STRALLOC( ( char * )sender );



const char, char * and casts are not something i know much(anything) about, but i would like to know more, i think about the only thing i think i know is that you use a const when whats being stored is not expected to change.

Heh, this might be a good topic for one of Nicks Legendary Tutorials.

You don't mean that backwards do you? Why would it complain arg1 is never NULL, if you are comparing to \0?

NULL and \0 are really quite different things.

• NULL is technically a "pointer to nothing" which is often implemented as a pointer containing the address zero.
  
  See: http://c-faq.com/null/varieties.html
  and: http://c-faq.com/null/macro.html
  
• The representation \0 refers to the value zero. As the FAQ above says you should only use NULL for pointers and not values. As an example:
  
  

  
  char * p;
  
  p = malloc (10);
  
  if (p != NULL)
    *p = '\0'; 
  

  
  
  Internally NULL and \0 may well both refer to zero however one is a pointer and one is the value the pointer is pointing to.

so, if ( arg1[0] == '\0' ) would be the correct way to do this and if ( arg1 == NULL ) would be incorrect? because arg1 is not a pointer. In the above instance its a char.

Sorry Nick if i seem slow, just making sure that i totally understand what your saying.

Im going now to read those 2 faqs.

As a little analogy, if I asked you "how many apples do you have?" and you answered "zero oranges", you might argue that the reply is OK because zero is zero, but I might reply that you have responded with the wrong "type" of fruit.

I am supposing you mean an array of char, because otherwise you can't use the square brackets.

If you have this:



That is OK, you are testing one of the bytes inside arg1.

However:



Is completely meaningless because arg1 is not a pointer and thus is never NULL.

However consider this:



This is fine, because malloc can fail to allocate memory, in which case NULL is returned. Notice the distinction - the first example used memory on the stack, which must exist, the last example used memory on the heap, which may or may not exist.

See this for an explanation:

http://c-faq.com/aryptr/aryptrequiv.html

Arhh see i did not know that \0 being is the Ascii null, which is what i should look for when working with strings. Yes your analogy clears this up perfectly, while both are zeros they are as different as apples and oranges.

And your post above this clarifies it even further.

Well, that's basically all there is to it, honest. :) It's a safety mechanism, and serves as a statement of intent. By touching a const variable, you are essentially entering a (bendable) contract to not mess with it. Of course, sometimes you need to strip the const annotation. There are two such situations:

(1) for some reason, you want to edit it, even though you're not supposed to. This is very, very unusual, and if it comes up, it might be worth rethinking why you're editing a const variable.

(2) much more frequent: you need to pass something to a function that you know doesn't edit it, but for whatever reason (usually laziness or not knowing any better on the part of the function writer) the function isn't marked as having const variables. Therefore you need to temporarily remove the const keyword with a case.

Of course, in case (2), the compiler (rightfully) complains at you, telling you that you're doing something dangerous. For all you know, that function might modify the data, which is something it really shouldn't do -- and that modification might even cause a crashing bug. (If the data is read-only, and that read-onliness is enforced by the operating system, attempting to write will cause a runtime error.)



I think Nick's explanation of the arg question is very fine so I'll not elaborate on that point. :) I'll just highlight one point which is the conceptual difference between null and zero. Java makes this difference explicit, actually, by marking it a compile-time error to assign null to non-pointers, or zero to pointers.

To be honest, this sort of stuff confuses even experienced programmers, and the C FAQ is a great resource for clearing these things up.

Some of the confusion has arisen because of C's historical treatment of dynamically allocated memory, and arrays of things, as very similar. We often see examples of people trying to do something with static arrays that is best (or only) done with dynamically allocated memory - as an example, you can't "free" a static array.

I think what is happening is that compiler-writers are getting a bit more strict in their error (or warning) checks, with the good intention of helping people detect code that looks good, but is meaningless, or not achieving what you think it is.

note->sender = STRALLOC( ( char * )sender );

So then with this line its it STRALLOC that does not have const variables, so it needs the cast, which brings up the warning to i can check to make sure what im doing is correct.

Why then does C not come with a version of STRALLOC that has const varialbes so that i could just,

note->sender = STRALLOC( sender );

Well it isn't C, it is SMAUG, which implements STRALLOC.

In mud.h:



This means STRALLOC is really synonymous with str_alloc.

Now in hashstr.c we see:



This looks kind of creepy to me. The prototype (first line) defines the input string as non-const, however the implementation (a few lines further) has it defined as const. I would have expected a warning here. I would change:



to:



However that possibly might make other code generate warnings, however it didn't with my version.

This time i am almost 1 step ahead of you, i was just looking over the macros for STRALLOC and friends and working out if i was using the HASH version.

Ill post back in a min with the results of changing what you suggested.

Ok, i did not have that mismatch like you did nick, both the prototype and the function were both char * str.

I made both const char *, changed this

note->sender = STRALLOC( ( char * )sender );

to this

note->sender = STRALLOC( sender );

and now it gives this warning,

board.c:908: warning: passing argument 1 of ‘str_alloc’ discards qualifiers from pointer target type

which i can only assume that it has to do with the macro,
#define STRALLOC(point) str_alloc((point))

the ((point)) does that mean that point is being cast? and would changing it to (point) made any difference.

I found it,
char * str_alloc args( ( char *str ) );
in mud.h is what was changing it, can
char * str_alloc args( ( const char *str ) );

i did this and now
note->sender = STRALLOC( sender );

compiles without warnings, but is it correct what i did, or could this actually induce more problems for me than just correcting 1 warning on a cast.

Ah, so they're finally starting to slip 4.2 into more common distros eh? Lovely. I've been looking to avoid it myself since a lot of these kinds of warnings are going to cause grief on an epic scale. Code that "worked fine" before suddenly "breaks" and has to be "fixed". It's gcc3 syndrome all over again :)

I forced the const errors to the surface in AFKMud by using the write-strings compiler flag and ended up giving up and going with std::string because the whole thing was just being ridiculous about the const flag.

Well, it's actually doing you a big favor, from a certain point of view... If you recall that social bug from the FUSS forums, that was almost solely due to not understanding and therefore mishandling const strings. In the end of the day the compiler is just strongly encouraging people to actually program properly and not be lazy or ignorant about const. Sure, it causes some grief, but how much grief has been caused by screwing up on subjects that the compiler now warns you about?


This is fine; it was seeing the declaration in mud.h, not the prototype in hashstr.c. So, it only knew about the non-const version.

I understand the reason for making the change, but in this case the inconvenience was non-trivial for a whole lot of people. Not just us sloppy MUD coders this time :)

I turned up Google hits from all sorts of angry project leaders, up to and including folks working on the Gnome and KDE projects. It may be proper behavior but it's likely slowing the adoption of 4.2.x more than normal as all these people need to stop and fix stuff rather than getting real productive work done.

For MUD code, I suspect there's going to be some major headaches on the horizon since Smaug, Rom, Godwars, Merc, Circle, and most of the others will run into the same kind of roadblocks that drove me to using std::string. It may well be inevitable that the past catches up to us, but it would have been nice for it to catch up with less interference.

I think that the past always catches up to people with heaps of interference; I think that's almost the definition of the past catching up to you. ;-)

Do you mind elaborating further? what sort of issues should i be aware of, what future troubles possibly lie in wait within smaug code?

SMAUG (and similar) code is notorious for having, err, somewhat sketchy code constructs. Some of these have already started cropping up as common errors, such as the 'invalid lvalue cast' one that comes up from time to time. SMAUG also uses const very poorly, and in fact generally doesn't use it at all. This makes life very complicated when you want to start doing things properly with 'const' in functions like one_argument, because it makes every calling location report an error unless it was using const (and most of them don't) -- but then fixing those just causes the error (or warning) to propagate backwards further.

I still think that forcing people to program correctly is a good thing, because many bugs are caused by things like this -- mishandling of strings, for instance, that wouldn't have happened had they been properly const'ed. But Samson is right in that this will cause all kinds of trouble for fixing it. It's a case where the small cost way back when to do things the right way would have prevented the very large cost most people are going to be faced with now if they upgrade gcc versions.

</soapbox> :-)