Tzafrir Cohen
Sun, 29 Jul 2001 03:36:02 -0700
On Sun, 29 Jul 2001, mulix wrote:
> r2llib.c:
> =========
>
> In function r2l_init():
> -----------------------
> r2llib.c:40:
> Line 40: I think the code here should check if DEFAULT_FILE_NAME is not
> an absolute pathname.
>
> mulix: DEFAULT_FILE_NAME is defined, by me, on line 11. i know it's not
> an absolute pathname. in general, if we allow getting the
> DEFAULT_FILE_NAME from the environment, you are right.
>
> r2llib.c:41:
> Line 41: I would replace
>
> } else {
> if (fname[0] != '/'){
>
> with:
> } else if (fname[0] != '/') {
> but it's just style.
>
> mulix: i'm from the 'brace everything' school of thought. anything else
> is prone to errors.
>
> r2llib.c:44:
> Line 44: rtfm-ing getcwd(), it looks like lazy people like you (-:and I :-)
> can pass 0 for the size parameter. getwd() might fail. In this case I think
> it is better if r2llib_init() fails instead of going on attempting to use
> fname without the prefix.
>
> mulix: passing 0 or -1 for the size paraemeter did not work asexpected
> on my machine, so i opted for the current code. as for what happens if
> getcwd() fails, i think we should strive to go on even in the face of
> disaster - consider that r2llib failing means biditext fails - no hebrew
> at all for this application. when we should fail and when not to is open
> for debate... so, let's debate :)
This is a good approach for biditext, but not for a simple r2l program.
My feeling is that there are two seperate init functions needed, one for
interactive programs, with argv, and that may fail, and one for a daemon
with no argv that should not cause any catastrophy.
> parse_argv.c:29:
> File parse_argv.c
> =================
>
> In function parse_command_line()
> --------------------------------
>
> I would make this function check and delete the --file-name argumentonly
> if it is the first one. Otherwise this might interfere with programs that
> accept a similar argument. E.g.:
>
> Assuming that r2ltest is linked with r2llib:
>
> r2ltest foo --file-name foo.in
>
> In this case --file-name should be interpreted as a parameter passed to
> foo and not to r2ltest. On the other hand:
>
> r2ltest --file-name /tmp/.rev foo
>
> means that in this case --file-name is passed to r2ltest.
>
> Also this example:
>
> r2ltest --file-name /tmp/.rev foo --file-name foo.in
>
> should interpret the first --file-name for r2ltest while leaving the
> other arguments alone (e.g. the second --file-name which should be passed
> to foo).
>
> I think this behavior is more reasonable and also simplifies the code. I
> must admit that I might be a little bitbiased to the case
> r2ltest == biditext.
But r2l may have some other arguments. Forcing --file-name to be the first
is not that good.
Maybe stop parsing wen you run into the argument "--", so the user will be
able to resolve conflicts ?
>
> mulix: a better option will be to use an unambigous file name option,
> such as --r2l-file-name. i'll fix this when i get back.
>
>
> file_ops.h:1:
> File file_ops.h
> ===============
> No comments.
>
> file_ops.c:52:
> File file_ops.c
> ===============
>
> In function delete_file():
> --------------------------
> Line 52: Since you used the UNIX system call in all other places, I think
> it's better to be consistent and use unlink() instead of remove().
>
> mulix: actually, i chose remove() because it will work even for the
> (mind boggling) case where someone makes a directory, and gives us the
> directory as the rev file name.
> biditext_base_state.c:56:
> In function get_biditext_base_state()
> -------------------------------------
>
> Line 56: I would check more carefully the values returned from file_size()
> (i.e. an explicit check for (-1, 0, 1, and 2)) and fail (BT_ERROR) if this is
> not the case.
>
> mulix: i disagree. tzafrir said (0|odd|even), not (0|1|2). the latter
> was my implementation decision. coding the checks for (0|1|2) would've
> been forcing my decisions on any othe implementation, coding the former
> is interoperable.
Just a comment here:
)|odd|even was not a grand design decision. It was the first thing I could
think of, and I mentioned it spesifically in my post. I thought that the
file size is generally easier to manipulate and more portable then, say,
modification time.
Just as an example, I once wanted to try running a biditext-ed mail
client, where the .rev file will actually be one of the folders (assuming
mbox or similar format) and thus it could be controlled from within the
mail client (by creating and removing this mail folder). I have no idea ow
to create an zero-sized mail folder, though.
(of course, 0|1|2 is less flexible)
--
Tzafrir Cohen
mailto:[EMAIL PROTECTED]
http://www.technion.ac.il/~tzafrir