Refactored InfoSink. I have replaced most instances of sprintf with std::ostr...

Refactored InfoSink. I have replaced most instances of sprintf with std::ostringstream to make it safer. I have made sure that everything still compiles and passes conformance tests.

[dgkoch, 2010-05-28 20:41:15]

looks good here.

[kbr1, 2010-06-01 19:59:54]

LGTM with a couple of comments.

http://codereview.appspot.com/1391041/diff/1/5
File src/compiler/InfoSink.h (right):

http://codereview.appspot.com/1391041/diff/1/5#newcode40
src/compiler/InfoSink.h:40: TInfoSinkBase() {}
It looks like there are differing line feed characters in this section of code.

http://codereview.appspot.com/1391041/diff/1/5#newcode93
src/compiler/InfoSink.h:93: const char* c_str() const { return sink.c_str(); }
This is unsafe if the TInfoSinkBase is destroyed. Can we delete this method and
its uses entirely from the code? For best safety (although it's inefficient)
str() would also return TPersistString rather than const TPersistString&,
although I'm not sure that change is worth it.

[Alok Priyadarshi, 2010-06-01 20:27:31]

http://codereview.appspot.com/1391041/diff/1/5
File src/compiler/InfoSink.h (right):

http://codereview.appspot.com/1391041/diff/1/5#newcode40
src/compiler/InfoSink.h:40: TInfoSinkBase() {}
It is just being shown that way by reitveld because of >80 chars in the old
code.

http://codereview.appspot.com/1391041/diff/1/5#newcode93
src/compiler/InfoSink.h:93: const char* c_str() const { return sink.c_str(); }
I agree. Deleting this method would require fairly intrusive refactoring of the
translator API though. The current API has the following two functions:
const char* ShGetInfoLog(const ShHandle);
const char* ShGetObjectCode(const ShHandle);
I think it should be changed to something similar to OpenGL shader API. The
client queries for info-log length; allocates a string; passes the allocated
string to get info-log. I will do it in a different CL if thats fine with you.

I think using const TPersistString& for str() is fine in this case. str() is
completely internal to shader translator. Also InfoSink is a member of
ShCompiler, so it will not get destroyed before the compiler itself.

[kbr1, 2010-06-01 20:35:36]

http://codereview.appspot.com/1391041/diff/1/5
File src/compiler/InfoSink.h (right):

http://codereview.appspot.com/1391041/diff/1/5#newcode40
src/compiler/InfoSink.h:40: TInfoSinkBase() {}
On 2010/06/01 20:27:32, alokp wrote:
> It is just being shown that way by reitveld because of >80 chars in the old
> code.

I see. That's fine.

http://codereview.appspot.com/1391041/diff/1/5#newcode93
src/compiler/InfoSink.h:93: const char* c_str() const { return sink.c_str(); }
On 2010/06/01 20:27:32, alokp wrote:
> I agree. Deleting this method would require fairly intrusive refactoring of
the
> translator API though. The current API has the following two functions:
> const char* ShGetInfoLog(const ShHandle);
> const char* ShGetObjectCode(const ShHandle);
> I think it should be changed to something similar to OpenGL shader API. The
> client queries for info-log length; allocates a string; passes the allocated
> string to get info-log. I will do it in a different CL if thats fine with you.

That's fine.

> I think using const TPersistString& for str() is fine in this case. str() is
> completely internal to shader translator. Also InfoSink is a member of
> ShCompiler, so it will not get destroyed before the compiler itself.

I see. Sounds fine.