Refactored ReadFileData() function in the translator sample to fix many issue...

Refactored ReadFileData() function in the translator sample to fix many issues:
- Memory leaks
- Made compiling using multiple strings default. This was not getting exercised.
- Removed redundant copies of file data
- Handled empty files properly which were getting ignored
BUG=66

[Alok Priyadarshi, 2010-10-19 19:34:36]

This is in preparation to fix BUG 66.
http://code.google.com/p/angleproject/issues/detail?id=66

[kbr1, 2010-10-19 19:59:56]

This basically LGTM but I have one question and one suggestion.

http://codereview.appspot.com/2579041/diff/1/samples/translator/translator.cpp
File samples/translator/translator.cpp (right):

http://codereview.appspot.com/2579041/diff/1/samples/translator/translator.cp...
samples/translator/translator.cpp:225: int len = (int)(ceil)((float)count /
(float)MAX_SOURCE_STRINGS);
Why is ceil parenthesized?

http://codereview.appspot.com/2579041/diff/1/samples/translator/translator.cp...
samples/translator/translator.cpp:230: int nread = fread(return_data[i], 1, len,
in);
What about the possibility that we didn't read "len" bytes this time around? We
may end up silently dropping data from the end of the file. Why not just use a
std::vector<char*> and convert it to a char** at the end of the operation?

[Alok Priyadarshi, 2010-10-19 20:20:06]

http://codereview.appspot.com/2579041/diff/1/samples/translator/translator.cpp
File samples/translator/translator.cpp (right):

http://codereview.appspot.com/2579041/diff/1/samples/translator/translator.cp...
samples/translator/translator.cpp:225: int len = (int)(ceil)((float)count /
(float)MAX_SOURCE_STRINGS);
On 2010/10/19 19:59:56, kbr1 wrote:
> Why is ceil parenthesized?

Done.

http://codereview.appspot.com/2579041/diff/1/samples/translator/translator.cp...
samples/translator/translator.cpp:230: int nread = fread(return_data[i], 1, len,
in);
On 2010/10/19 19:59:56, kbr1 wrote:
> What about the possibility that we didn't read "len" bytes this time around?
We
> may end up silently dropping data from the end of the file. Why not just use a
> std::vector<char*> and convert it to a char** at the end of the operation?

May be I do not understand your concern. The usage of ceil makes sure that the
file data fits into MAX_SOURCE_STRINGS bins. The last bin may have fewer chars
than the preceding ones. The possibility of not reading "len" bytes is handled
through nread variable.

[kbr1, 2010-10-19 20:45:22]

On 2010/10/19 20:20:06, Alok Priyadarshi wrote:
> http://codereview.appspot.com/2579041/diff/1/samples/translator/translator.cpp
> File samples/translator/translator.cpp (right):
> 
>
http://codereview.appspot.com/2579041/diff/1/samples/translator/translator.cp...
> samples/translator/translator.cpp:225: int len = (int)(ceil)((float)count /
> (float)MAX_SOURCE_STRINGS);
> On 2010/10/19 19:59:56, kbr1 wrote:
> > Why is ceil parenthesized?
> 
> Done.
> 
>
http://codereview.appspot.com/2579041/diff/1/samples/translator/translator.cp...
> samples/translator/translator.cpp:230: int nread = fread(return_data[i], 1,
len,
> in);
> On 2010/10/19 19:59:56, kbr1 wrote:
> > What about the possibility that we didn't read "len" bytes this time around?
> We
> > may end up silently dropping data from the end of the file. Why not just use
a
> > std::vector<char*> and convert it to a char** at the end of the operation?
> 
> May be I do not understand your concern. The usage of ceil makes sure that the
> file data fits into MAX_SOURCE_STRINGS bins. The last bin may have fewer chars
> than the preceding ones. The possibility of not reading "len" bytes is handled
> through nread variable.

If enough of the freads read fewer bytes than expected, then the last bin will
not be able to fit the amount it was intended to read plus the amounts dropped
from the previous bins.

[Alok Priyadarshi, 2010-10-19 21:02:25]

>
> If enough of the freads read fewer bytes than expected,
>

This only happens in two cases:
1. EOF is reached, which is handled by the "break" statement.
2. An error occurred while reading the file - I can add ferror to
guard against this if you want.

[kbr1, 2010-10-19 22:55:33]

On 2010/10/19 21:02:25, Alok Priyadarshi wrote:
> >
> > If enough of the freads read fewer bytes than expected,
> >
> 
> This only happens in two cases:
> 1. EOF is reached, which is handled by the "break" statement.
> 2. An error occurred while reading the file - I can add ferror to
> guard against this if you want.

What if the file is network mounted and fread can't return all of the bytes
requested? It seems to me there are plenty of reasons, code simplicity included,
to just use std::vector<char*> for this loop rather than preallocating a certain
number of bins.

[Alok Priyadarshi, 2010-10-20 18:11:10]

changed to use std::vector<>.

[dgkoch, 2010-10-20 19:43:14]

no issues here.

BTW is this the actual translator, or just sample code showing how to use the
translator?

Daniel

[Alok Priyadarshi, 2010-10-20 19:44:33]

On 2010/10/20 19:43:14, dgkoch wrote:
> no issues here.
> 
> BTW is this the actual translator, or just sample code showing how to use the
> translator?
> 
> Daniel

Just sample code. It resides under samples/ directory.