cosmoharrigan / rl-glue

Automatically exported from code.google.com/p/rl-glue
0 stars 0 forks source link

env_init() returning task_specification_t #98

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Hi,

Very minor issue.

I believe that this method should return a "const task_specification_t"
rather than a normal one. That is assuming, of course, the internals of
rl-glue make no changes to it.

This would allow developers (me) to return s.c_str() at the end of this
method, where s is of type std::string. I think that it is good practice,
in general, to use type const char* whenever the string is only read from,
and never written to.

- John

Original issue reported on code.google.com by jasm...@gmail.com on 8 Oct 2008 at 7:55

GoogleCodeExporter commented 9 years ago
Good question.  If I remember, Andrew Butcher (henceforth "The Butcher") and I 
had a
fight about this about a year ago.

I can't remember who took what side, but we ended up deciding not to do it.  I 
can't
remember the exact details, unfortunately.  It was something about how C and C++
handled const return types differently, and it was giving us errors or warnings 
that
we couldn't shake.

I'm going to check into it again actually.

The reason we didn't pursue the idea too hard was that it didn't matter.  Const
correctness is not enforceable in C/C++, so the compile cannot optimize for it, 
so
its sometimes not worth the bother. 

Before I go on this tangent: please accept my apologies if you know all this 
stuff
already.  I didn't really when we first ran into some of this stuff.

For example, this code is *totally* valid:

    std::string myString("SAFE! is good news!");
    const char* safeString=myString.c_str(); //still safe
    char* unsafeString=(char *)safeString;//uhoh
    memcpy(unsafeString,"PWNED",5);//what did that do
    printf("safeString is: %s\n",safeString);

Output: safeString is: PWNED is good news!

So the lesson here is that you can override these return types anyways.

You can just do:
return (char *)myCPPString.c_str();

However, beware!  If these are stack-based variables, this is a very dangerous 
idea.
 If you are creating the string inside of env_init() as a stack variable, then that
variable gets popped off the stack right after you return and the validity of 
that
pointer is not well defined.  Things might be ok some of the time, but other 
times
you might be reading random memory.

Better to use a pointer to a string, or to keep a buffer around either as a 
global
variable or on the heap, and then do something like:
strcpy (myGlobalBuffer, myCPPString.c_str());
return myGlobalBuffer;

Original comment by brian.ta...@gmail.com on 9 Oct 2008 at 5:44

GoogleCodeExporter commented 9 years ago
This ended up being handled in the big changeover to const pointers instead of 
structs.

Still wish I had got more feedback on that change.

Original comment by brian.ta...@gmail.com on 15 Oct 2008 at 1:53

GoogleCodeExporter commented 9 years ago
Well, I didn't even know that change happened! But now I do, and it seems fine. 
After
some typing I was able to get my stuff to work with the newer RC just fine.

Original comment by jasm...@gmail.com on 15 Oct 2008 at 7:43