The Design of Software (CLOSED)

A public forum for discussing the design of software, from the user interface to the code architecture. Now closed.

The "Design of Software" discussion group has been merged with the main Joel on Software discussion group.

The archives will remain online indefinitely.

Code complication to avoid warnings

The following fragment of C code is said to throw a compiler warning by a colleague - though they haven't provided the actual warning text - something along the lines of
was getting 38 warnings, one for each line where sprintf is given a char*[5] rather than a char*...
however the code below represents the problem, and that a char[5] is being used.

char theString[5];
populateTheString(theString);
sprintf(outputStr, "%s\n", theString);


This can be easily fixed by modifying the sprintf statement to:
sprintf(outputStr, "%s\n", &theString[0]);

But this is quite a bit uglier than the former statement.

The question my colleague has is: Would people prefer more confusing code that compiles without warnings, or nicer code that produces a large number of warnings when it is compiled?

My question is, why is the compiler putting out such a dodgey message on code when he compiles that I think should be perfectly fine, and other samples do work fine.

Compiler is MSVC V6
Gorf Send private email
Wednesday, July 06, 2005
 
 
Warnings are very wierd sometimes. Just move the code around and they can go away.

But to answer your question, I would always prefer to compile without warnings, even if I had to do whack things.

In your case this looks normal to me:
sprintf(outputStr, "%s\n", &theString[0]);

But the code is crap anyway. There is no protection against buffer overflow.

I think there's a way to be safe, have good code, and get rid of the warnings. I would start by not embedding sprintf in anything but the lowest level library function.
son of parnas
Wednesday, July 06, 2005
 
 
I would prefer slightly weird code and no warnings.

If you've got a program that produces 300 warnings, it's next to impossible to weed out that that are harmless and those that point to serious problems.
Chris Tavares Send private email
Thursday, July 07, 2005
 
 
So in MSVC terms, your happy to #pragma the warning out of existance to hide it, or to rearrange the code, even if the compiler is being silly, or perhaps there is a real problem, like the variable is being overloaded and you didn't notice?
Gorf Send private email
Thursday, July 07, 2005
 
 
I always use 'warnings as errors'. Wierd constructions show intent.
Mr Jack
Thursday, July 07, 2005
 
 
Either the compiler is wrong, or your code fragment is not representative, because the type of the argument (according to your code) is char[5], which decomposes into char *, and is thus perfectly suitable as an input for %s.

"&theString[0]" and "theString" are effectively synonymous. And by the same token, if you had "char *theString[5]" (for which the compiler would be quite justified in giving a warning about the type being char *[5] not char *), "&theString[0]" would be no less incorrect than "theString".

Regarding warnings, I always change the code to get rid of them, because they're pretty handy -- VC++ and gcc have a few that are almost universally pointless, but you can work around just about all of them without having to disable any. (Except, as I recall, "class has private destructor but no friends" -- terribly annoying, especially as this was exactly what I wanted.)
Tom_
Thursday, July 07, 2005
 
 
I second Tom_'s doubts about this.  Not only does it compile without warnings for me, but I don't see how it's possible for this code snippet to give the indicated warning; MSVC 6 doesn't check the types of variadic arguments.
Jonas Grumby Send private email
Thursday, July 07, 2005
 
 
If the compiler is worth anything, it is likely complaining about a naked call to sprintf() (without using a precision specifier in your format string), which is equivalent to begging for a buffer overflow.

To answer your original question, I think you should turn on whichever warnings are deemed appropriate, flag warnings as errors, and fix all resulting warnings encountered in the code.

Thursday, July 07, 2005
 
 
Personally, I don't complicate the code to get rid of warnings.  However, I am largely working in C#, not C or C++.  I think this comes about because in C# there are warnings that are incorrect.  An example in C#:

protected int filetypeid;
protected bool filetypeidchanged;

public int FILETYPEID {
  get{ return filetypeid;}
  set{
    filetypeid = value;
    filetypeidchanged = true;
    OnFILETYPEIDChanged();
  }
}

Get me a warning about filetypeidchanged never being used.  Something like:

C:\Documents and Settings\User\Desktop\SM\Volz Software\VSD\Containers\Search_FileTypeContainer.cs(40,14): warning CS0169: The private field 'VSD.Containers.Search_FileTypeContainer.filetypeidchanged' is never used

/*Okay so I checked into this, and it appears that this warning only appears to happen some of the time, which is strange, since this code is being written by a code generator, so the exact same code (different variables names) produces warnings in some places, but not in others.*/

Additionally, I used to use a product from Parasoft that checked code and gave me warnings based on the Microsoft best practices (they had a rules engine that did the comparisons).  In some cases, I would discover that Microsoft was breaking their own best practice rules.  I checked to make sure that the testing software wasn't applying the rule incorrectly (they give you a link to where Microsoft states the best practice).  As it turned out, Microsoft was indeed not following their own best practices (specifically, they use readonly, a keyword in C#, as a variable name with different capitalization). 

I am not sure if this helps, but I thought I would put it out there for discussion.

Josh
Joshua Volz Send private email
Saturday, July 09, 2005
 
 
Well, in the code you gave, filetypeidchanged is never used. It's assigned, but never referenced, and therefore does nothing.

I'll just assume there's more further down you didn't paste in. ;-)
Chris Tavares Send private email
Sunday, July 10, 2005
 
 
I use a compiler (targeting a small microcontroller) that gives a warning for the same thing and my solution was to introduce the ugly syntax hack to get rid of it. Any developer worth their salt will realize that the expressions evaluate to the same thing. My advise: get rid of the warning messages. I hate having to scan through a list of warning messages that I have been "assured" don't mean anything.

With respect to:

"If the compiler is worth anything, it is likely complaining about a naked call to sprintf() (without using a precision specifier in your format string), which is equivalent to begging for a buffer overflow."

This is incorrect. The compiler should not be checking  the value of arguments in function calls. That is the developer's job.
A. Nonymous
Tuesday, July 12, 2005
 
 
"My question is, why is the compiler putting out such a dodgey message on code when he compiles that I think should be perfectly fine, and other samples do work fine."

Compiler bug. It happens to the best of us, too :)
A. Nonymous
Tuesday, July 12, 2005
 
 
"This is incorrect. The compiler should not be checking  the value of arguments in function calls. That is the developer's job."

The compiler is a program that does whatever is convenient and useful in its domain.  It is very useful to make bad or unsafe code not compile or compile with warnings.  The developers tend to be much worse than the compiler at consistently spotting things like this *if* the compiler can be taught to recognize it at all (not necessarily true).  Lets build on each others strengths instead of arbitrarily assigning "jobs" to the entities involved.
Haertchen
Wednesday, July 13, 2005
 
 

This topic is archived. No further replies will be accepted.

Other recent topics Other recent topics
 
Powered by FogBugz