Well, Thanks for clarifying for me. :D

Now, I just need to get my code to compile. I obviously don't know the ins and outs of this as well as I thought I did.

I'm gonna call it a night for now, But I'm probably going to need some help getting this figured out and working with some of my changes.

Alright, Well, I've tried and tried off and on all day as I've had time to sit down and work on it, and I'm not really getting it. I'm also finding a couple places where maybe it should have been changed but wasn't. I've just been casting as (char *) in those places, but those places seem to be, for me at least, calls to ch_printf( );.

If it's not too much trouble, David, could you explain for me why these changes were needed exactly, and how I need to go about writing my code now to stay within the standards? Also, please note, that I'm mostly self-taught so I don't know all the lingo.

There are so many changes that it would take a really long time to go through them all... The basic idea is that any time a function takes a string argument that it has no business changing, that string should be const. You probably shouldn't be casting since that is a sign that the functions in question don't have a correct prototype. Can you give an example of where you're casting and what the function prototype looks like?

Yeah, I can give some examples.

if( !clan->emote[x] )
            ch_printf( ch, ( char * )chan_data[channel].buf_text, capitalize( verb ), buf, speaks
                       ? buf2 : scramble( buf2, clan->histlang[x] ) );

This is from my channels code, I had to cast it as a (char *) to get ch_printf to accept it without complaint. Here's the channel struct:

struct channel_data
{
   int channel;
   const char *prefix;
   const char *act_text;
   const char *buf_text;
   int color;
   int type;
   char_data *speaker[MAX_CHANHISTORY];
   const char *argument[MAX_CHANHISTORY];
   const char *timestamp[MAX_CHANHISTORY];
   int visibility[MAX_CHANHISTORY];
   int invislevel[MAX_CHANHISTORY];
   int language[MAX_CHANHISTORY];
   bool emote[MAX_CHANHISTORY];
};

And what that pans out to for the channels:

struct channel_data chan_data[] = {
    { CHANNEL_CHAT,       "&W[&[chat]%s&W]&[chat] %s", "&W[&[chat]%s&W] &w$n&W:&[chat] $t", "&W[&[chat]%s&W] &w%s&W:&[chat] %s\n\r", AT_GOSSIP, 1 },
    { CHANNEL_WARTALK,    "&W[&[wartalk]%s&W]&[wartalk] %s", "&W[&[wartalk]%s&W] &w$n&W:&[wartalk] $t", "&W[&[wartalk]%s&W] &w%s&W:&[wartalk] %s\n\r", AT_WARTALK, 1 },
    { CHANNEL_IMMTALK,    "&W[&[immortal]%s&W]&[immortal] %s", "&W[&[immortal]%s&W] &w$n&W:&[immortal] $t", "&W[&[immortal]%s&W] &w%s&W:&[immortal] %s\n\r", AT_IMMORT, 2 },
    { CHANNEL_AVTALK,     "&W[&[immortal]%s&W]&[immortal] %s", "&W[&[immortal]%s&W] &w$n&W:&[immortal] $t", "&W[&[immortal]%s&W] &w%s&W:&[immortal] %s\n\r", AT_IMMORT, 2 },
    { CHANNEL_RACETALK,   "&W[&[racetalk]%s&W]&[racetalk] %s", "&W[&[racetalk]%s&W] &w$n&W:&[racetalk] $t", "&W[&[racetalk]%s&W] &w%s&W:&[racetalk] %s\n\r", AT_RACETALK, 1 },
    { CHANNEL_HIGHGOD,    "&W[&[muse]%s&W]&[muse] %s", "&W[&[muse]%s&W] &w$n&W:&[muse] $t", "&W[&[muse]%s&W] &w%s&W:&[muse] %s\n\r", AT_MUSE, 2 },
	{ CHANNEL_HIGH,       "&W[&[think]%s&W]&[think] %s", "&W[&[think]%s&W] &w$n&W:&[think] $t", "&W[&[think]%s&W] &w%s&W:&[think] %s\n\r", AT_THINK, 2 },
    { CHANNEL_CLAN,       "&W[&[clantalk]%s&W]&[clantalk] %s", "&W[&[clantalk]%s&W] &w$n&W:&[clantalk] $t", "&W[&[clantalk]%s&W] &w%s&W:&[clantalk] %s\n\r", AT_CLANTALK, 1 },
    { CHANNEL_GUILD,      "&W[&[guildtalk]%s&W]&[guildtalk] %s", "&W[&[guildtalk]%s&W] &w$n&W:&[guildtalk] $t", "&W[&[guildtalk]%s&W] &w%s&W:&[guildtalk] %s\n\r", AT_ORDERTALK, 1 },
    { CHANNEL_ORDER,      "&w[&[ordertalk]%s&w]&[ordertalk] %s", "&w[&[ordertalk]%s&w] &W$n&w:&[ordertalk] $t", "&w[&[ordertalk]%s&w] &W%s&w:&[ordertalk] %s\n\r", AT_GUILDTALK,  },
    { CHANNEL_TRIBUNAL,   "&W[&[counciltalk]%s&W]&[counciltalk] %s", "&W[&[counciltalk]%s&W] &w$n&W:&[counciltalk] $t", "&W[&[counciltalk]%s&W] &w%s&W:&[counciltalk] %s\n\r", AT_COUNCILTALK, 1 },
    { CHANNEL_ARCHITECT,  "&W[&[counciltalk]%s&W]&[counciltalk] %s", "&W[&[counciltalk]%s&W] &w$n&W:&[counciltalk] $t", "&W[&[counciltalk]%s&W] &w%s&W:&[counciltalk] %s\n\r", AT_COUNCILTALK, 1 },
    { CHANNEL_FAMILIARUM, "&W[&[counciltalk]%s&W]&[counciltalk] %s", "&W[&[counciltalk]%s&W] &w$n&W:&[counciltalk] $t", "&W[&[counciltalk]%s&W] &w%s&W:&[counciltalk] %s\n\r", AT_COUNCILTALK, 1 },
    { CHANNEL_MUSIC,      "&W[&[chat]%s&W]&[chat] %s", "&[chat]$n %ss '$t'", "&[chat]%s %s '%s'\n\r", AT_GOSSIP, 1 },
    { CHANNEL_NEWBIE,     "&W[&[chat]%s&W]&[chat] %s", "&[chat]$n %ss '$t'", "&[chat]%s %s '%s'\n\r", AT_GOSSIP, 1 },
	{ -1,                 "&W[&[chat]%s&W]&[chat] %s", "&[chat]$n %ss '$t'", "&[chat]%s %s '%s'\n\r", AT_GOSSIP, 1 }
};

Every instance where I've tried to pass the channels through ch_printf so far have needed to be cast as (char *) before they'll get past -Wwrite-strings. But as soon as I cast them, they go through just fine. There's a bunch of other errors as well from other things, including do_group but I haven't gotten to that point yet. Hurt my shoulder today, been dealing with that and wedding stuff, so I haven't had a whole lot of time to work on much of anything today. Let me know if you need to see anything else.

[Edit:] I didn't mean each individual change, I merely meant in general why were the changes needed, what purpose it serves, etc. I'd like to better understand why this was needed so that I can maybe start to grasp strings a little better.

What's the prototype for ch_printf? Chances are that the string argument is a char* when it really should be a const char*.

(EDIT: and I'll answer the other question tomorrow, it's late and I'm tired )

The prototype, and the actual function for me are both still:

void ch_printf( char_data * ch, char *fmt, ... )

I don't recall seeing anywhere to change it in the diff, but that was a very long diff and a lot of changes, so I may very well have missed it. I'll double check the diff.

That one may not have been in the diff because the color code is where that function is at now and it might have already been given a const char* update. This is what your ch_printf should look like:

void ch_printf( CHAR_DATA * ch, const char *fmt, ... )
{
   char buf[MAX_STRING_LENGTH * 2];
   va_list args;

   va_start( args, fmt );
   vsnprintf( buf, MAX_STRING_LENGTH * 2, fmt, args );
   va_end( args );

   send_to_char( buf, ch );
}

Yep, updated that, took away the casts, and lo and behold, but not only did my problems with that go away, but a bunch of others in the file did too. Certainly looks a lot less daunting now anyway. I'll tackle more of that after a bit of shut eye though.

Yup, Samson got it.

To answer your other questions...

As you know strings in C are just a sequence of bytes in memory terminated by a 0 byte. A "string" is a pointer to the first such byte in a sequence. Of course, that means that a char* is (somewhat) ambiguous, in that it literally means a pointer to a byte in memory, but it is also how strings are denoted.

Because of the nature of pointers, any function that takes a char* and can modify it will be affecting the caller. Imagine if any time you passed an int, the callee could mess with the number and it would change for the caller as well. Much confusion could ensue, especially when you have a large, complex system with many functions calling each other.

The point of the const keyword is to help alleviate these problems. It is an indication both to the compiler and to the programmer that the function will not modify the value passed in. So, if we have a const char*, it means that although we can read the bytes in question, the compiler won't let us write to them. This means that the caller has some guarantee that anything passed in to a function will stay the same. Having such guarantees is, again, very important when you have large systems and don't have time to look at every last function to see what it will do.

Constness also helps avoid bugs, such as the one with socials we saw some time ago. If you remember, a literal string was being passed in to a function that modified its argument. This is a problem because many OS's/compilers separate literal strings into what is called a program's data segment -- stuff that is common to all processes running that program, the data constants if you will; the separation allows things like shared memory -- and that segment is read-only. (If it weren't, programs could interfere with others sharing that memory.) So, when the OS sees you trying to write to the data segment, it throws a segmentation fault.

(Ever wonder where the term seg fault comes from? There you go -- same thing when you try to write to the code segment of the program, which is a similar idea except that it stores the code.)

In this case, constness fixes the problem naturally because, with this string literal, you can only call functions that promise not to modify the argument. In the past, compilers did not treat string literals as const char*; this is a mistake and was probably kept around for so long because so many programs used it, probably without even realizing they were perpetuating the mistake. But, as we know well by this point, gcc 4.2 changed that, and warnings are thrown when you try to treat a string literal as a char*.

So basically, the fix is to find every function that has no business changing its arguments, and make those arguments const. This allows you to treat strings that should be const as actually const. When you want to modify something, you have to do a little bit more work, but this is a safety feature because most of the time you really don't actually want to be modifying things willy-nilly.

I hope that answers your questions at least somewhat...

That cleared up a bit of the confusion, yes.

But not enough apparently. I've had to scrap all my changes and roll back to before I started these changes because obviously, I was way too tired to be doing something as large as this on as little sleep as I had been getting. But anyway, there was a particular part of my code, support for something not completely implemented, and partially borrowed from Keberus' SWFotE, that I wasn't really all that sure how to go about fixing for const char* correctness.

The code in question is the loop right after the comment about a fun gag trick:

CMDF( do_say )
{
   char buf[MSL];
   char speak[10], endbit[2], secbit[2], thirdbit[2];
   const char *sbuf;
   char_data *vch;
   int x;
   size_t y;
   EXT_BV actflags;
#ifndef SCRAMBLE
   int speaking = -1, lang;

   for( lang = 0; lang_array[lang] != LANG_UNKNOWN; lang++ )
      if( ch->speaking & lang_array[lang] )
      {
         speaking = lang;
         break;
      }
#endif

      if( !argument || argument[0] == '\0' )
      {
      if( IS_NPC( ch ) )
      {
         send_to_char( "Say what?", ch );
         return;
      }

      ch_printf( ch, "&cThe last %d things you heard said:\n\r", MAX_SAYHISTORY );

      for( x = 0; x < MAX_SAYHISTORY; x++ )
      {
         char histbuf[MSL];
         if( ch->pcdata->say_history[x] == NULL )
            break;
         one_argument( ch->pcdata->say_history[x], histbuf );
         ch_printf( ch, "&R[%s]%s\n\r", mini_c_time( atoi( histbuf ), ch->pcdata->timezone ),
                    ch->pcdata->say_history[x] + strlen( histbuf ) );
      }
      return;
     }

   if( IS_ROOM_FLAG( ch->in_room, ROOM_SILENCE ) )
   {
      send_to_char( "You can't do that here.\r\n", ch );
      return;
   }

   //Snip out MW-Specific code
    
   actflags = ch->act;
   if( IS_NPC( ch ) )
       xREMOVE_BIT( ch->act, ACT_SECRETIVE );
   for( vch = ch->in_room->first_person; vch; vch = vch->next_in_room )
   {
      sbuf = argument;

      if( vch == ch )
         continue;

      /*
       * Check to see if character is ignoring speaker 
       */
      if( is_ignoring( vch, ch ) )
      {
         /*
          * continue unless speaker is an immortal 
          */
         if( !IS_IMMORTAL( ch ) || get_trust( vch ) > get_trust( ch ) )
            continue;
         else
         {
            set_char_color( AT_IGNORE, vch );
            ch_printf( vch, "You attempt to ignore %s, but" " are unable to do so.\r\n", ch->name );
         }
      }

#ifndef SCRAMBLE
      if( speaking != -1 && ( !IS_NPC( ch ) || ch->speaking ) )
      {
         int speakswell = UMIN( knows_language( vch, ch->speaking, ch ),
                                knows_language( ch, ch->speaking, vch ) );

         if( speakswell < 75 )
            sbuf = translate( speakswell, argument, lang_names[speaking] );
      }
#else
      if( !knows_language( vch, ch->speaking, ch ) && ( !IS_NPC( ch ) || ch->speaking != 0 ) )
         sbuf = scramble( argument, ch->speaking );
#endif
      sbuf = drunk_speech( sbuf, ch );

      MOBtrigger = false;

    // Fun gag trick. Heh. Mmf mmmMmmf mmmf.

      if( ch && xIS_SET( ch->act, PLR_GAGGED ) )
      {
         for( y = 0; y < strlen( sbuf ); y++ )
         {
            if( y == 0 )
            {
               sbuf[y] = 'M';
               continue;
            }
            if( isspace( sbuf[y] ) )
            {
               if( y != 0 )
                  if( y >= 2 && !isspace( sbuf[y - 2] ) )
                     sbuf[x - 1] = 'f';
               continue;
            }
            if( number_range( 1, 7 ) == 3 )
               sbuf[y] = 'M';
            else
               sbuf[y] = 'm';

            if( y == strlen( sbuf ) )
               sbuf[y] = 'f';
         }
      }

    MOBtrigger = false;
      
	//Snip out some MW-Specific code
	    sprintf( speak, "say" );
     
      sprintf( buf, "$n %ss, '$t'", speak );
      if( !IS_AFFECTED( vch, AFF_DEAF ) )
         act( AT_SAY, buf, ch, sbuf, vch, TO_VICT );
	  else
		 act( AT_SAY, "You see $n's lips moving but you hear no sound come out...", ch, NULL, vch, TO_VICT );
      if( !IS_NPC( vch ) && !IS_AFFECTED( vch, AFF_DEAF ) )
         update_sayhistory( vch, ch, sbuf );
   }

   ch->act = actflags;
   MOBtrigger = false;

   	//snip out some MW-Specific code.
	    sprintf( speak, "say" );
   
   sprintf( buf, "You %s, '$T'", speak );
   
   if( !IS_AFFECTED( ch, AFF_DEAF ) )
      act( AT_SAY, buf, ch, NULL, sbuf, TO_CHAR );
   else
      act( AT_SAY, "You speak but you hear no sound come out...", ch, NULL, vch, TO_CHAR );

   if( !IS_NPC( ch ) && !IS_AFFECTED( ch, AFF_DEAF ) )
      update_sayhistory( ch, ch, sbuf );
      
   if( IS_ROOM_FLAG( ch->in_room, ROOM_LOGSPEECH ) )
   {
      snprintf( buf, MSL, "%s: %s", IS_NPC( ch ) ? ch->short_descr : ch->name, argument );
      append_to_file( LOG_FILE, buf );
   }

   mprog_speech_trigger( argument, ch );

   if( char_died( ch ) )
      return;

   oprog_speech_trigger( argument, ch );

   if( char_died( ch ) )
      return;

   rprog_speech_trigger( argument, ch );

   return;
}

It was complaining about modifying a read-only address, or something like that, I think, that was a couple hours ago now, and at the time I didn't feel like messing with it so I commented it out. But I figure since it's going to come up again, I might as well get to the bottom of it. Looking at it now, I think I understand what I need to do, which is copy the buffer, and modify it again, but I'm not 100% sure. If you need it:

#define CMDF( name ) extern "C" void (name)( char_data *ch, const char *argument )

(And yes, char_data is supposed to be lowercase. It's one of the two structs that I've succeeded in converting to a class...)

Just a quick question, why did you go about changing the CMDF stuff (Commands) to make argument constant anyways? The first time I attempted to fix this in LOP I did the same thing, but soon decided to go about it a different way and managed to get it to work fine differently. If you want to check out an unreleased copy of the next LOP release let me know and I'll get you a copy so you can take a look at it for ideas.

Because the do_function has no idea where its argument is coming from. It could be a descriptor's buffer, it could be a string literal, it could be a string somewhere else, it could be a string extracted from a mudprog... It doesn't own the pointer, and therefore has no business changing it. Instead of introducing hackery at the caller location to protect against a do_function modifying your string, I felt that it was safer, cleaner and saner to simply enforce that contract for every do_function. Very few commands need to edit the argument anyhow (most just peel off bits with one_argument), and in those very few cases, it's fine to just copy the string.

Well, Here we go with Take 2 of applying this patch by hand. All files have been reset to prior to take 1 (thank god for svn) and as soon as I take one scalding hot shower to clear my sinuses so I can breathe, I'm going to hammer this out.

Here goes nothing!

If you're willing to share your code, I'd be curious to see how the bzr technique would work. I'd take FUSS, apply your code as a patch, and then merge in the const fix. It might not work completely, but it might get an awful lot of the changes figured out...

Well, I've already started Take 2, but if Take 2 fails, I'd be willing to give that a try. Anything'd be better then starting over with a copy of FUSS with this already applied.

[Edit:] Well, assuming you could guarantee that no one else could get a hold of my code.

I wouldn't share it, if that's what you meant. I'm just curious to see if it would work in the first place...

Well, if it fails again as horribly as it did before, We'll be finding out.

Boy, talk about beating a dead horse.. >.> I ran into something I hadn't seen yet while adding a few things in and making them compatible with the new way of doing things. And the one I was working on happened to be Exodus' Build/Idea/Typo replacement snippet. Well, I worked out all the errors, and was left with a couple warnings:

  Compiling o/buidty.o....
cc1plus: warnings being treated as errors
buidty.cpp: In function âchar* bugSeverity(short int)â:
buidty.cpp:290: warning: deprecated conversion from string constant to âchar*â'
buidty.cpp:291: warning: deprecated conversion from string constant to âchar*â'
buidty.cpp:292: warning: deprecated conversion from string constant to âchar*â'
buidty.cpp:293: warning: deprecated conversion from string constant to âchar*â'
buidty.cpp:294: warning: deprecated conversion from string constant to âchar*â'
buidty.cpp:295: warning: deprecated conversion from string constant to âchar*â'
buidty.cpp: In function âchar* ideaLevel(short int)â:
buidty.cpp:828: warning: deprecated conversion from string constant to âchar*â'
buidty.cpp:829: warning: deprecated conversion from string constant to âchar*â'
buidty.cpp:830: warning: deprecated conversion from string constant to âchar*â'
buidty.cpp:831: warning: deprecated conversion from string constant to âchar*â'
buidty.cpp: In function âchar* typoLevel(short int)â:
buidty.cpp:1338: warning: deprecated conversion from string constant to âchar*â'
buidty.cpp:1339: warning: deprecated conversion from string constant to âchar*â'
buidty.cpp:1340: warning: deprecated conversion from string constant to âchar*â'
buidty.cpp:1341: warning: deprecated conversion from string constant to âchar*â'
make[1]: *** [o/buidty.o] Error 1
make: *** [all] Error 2

Now, Here's the offending code:

char * bugSeverity( short type )
{
  switch( type )
  {
    case 1:  return "Lesser";	  break;
    case 2:  return "Minor";	  break;
    case 3:  return "Major";	  break;
    case 4:  return "Severe";	  break;
    case 5:  return "Critical";	  break;
    default: return "Unknown??";	  break;
  }
}

char * ideaLevel( short type )
{
  switch(type)
  {
    case 1:  return "Good";		  break;
    case 2:  return "Great";	  break;
    case 3:  return "Phenomonal";  break;
    default: return "Unknown??";	  break;
  }
}

char * typoLevel( short type )
{
  switch(type)
  {
    case 1:  return "misspelled";  break;
    case 2:  return "misused";	  break;
    case 3:  return "offensive";	  break;
    default: return "Unknown??";	  break;
  }
}

Now, I shut the compiler up by doing this:

char * bugSeverity( short type )
{
  switch( type )
  {
    case 1:  return (char*)"Lesser";	  break;
    case 2:  return (char*)"Minor";	  break;
    case 3:  return (char*)"Major";	  break;
    case 4:  return (char*)"Severe";	  break;
    case 5:  return (char*)"Critical";	  break;
    default: return (char*)"Unknown??";	  break;
  }
}

char * ideaLevel( short type )
{
  switch(type)
  {
    case 1:  return (char*)"Good";		  break;
    case 2:  return (char*)"Great";	  break;
    case 3:  return (char*)"Phenomonal";  break;
    default: return (char*)"Unknown??";	  break;
  }
}

char * typoLevel( short type )
{
  switch(type)
  {
    case 1:  return (char*)"misspelled";  break;
    case 2:  return (char*)"misused";	  break;
    case 3:  return (char*)"offensive";	  break;
    default: return (char*)"Unknown??";	  break;
  }
}

But I doubt that's right, it also occured to me that changing the functions themselves to be const char * etc might work. But I want to know for sure which is right.

[Edit:]Ran into this problem with the AFKMud SQL helpfiles code as well. The difference there is that I can't go in and change the mysql_safe_query defines, so I had to cast to (char*) to shut the compiler up, I couldn't find any other way. >.<

Casting those to char* is a nonono, because that leaves open the possibility of changing those strings and bringing about a crash. The correct thing to do would be to change the return type to const char*.

What is the SQL code doing? You basically never want to cast string literals to char*, unless something extraordinary is going on.

   // Query for single result.
   state = mysql_safe_query( (char*)"SELECT * FROM game_helps WHERE help_keyword='%s'", argument );
   result = mysql_store_result( &myconn );

This is just one example, But I can't get to the prototype of mysql_safe_query to make it accept const char* as an argument. Would this be one of those places to do up a bit of operator overloading? and make one that takes const char* to shut the compiler up, but then have it just be a wrapper to the actual mysql_safe_query?