mulix
Sun, 29 Jul 2001 02:10:49 -0700
On Sun, 29 Jul 2001, Tzafrir Cohen wrote:
> Where can I find the review?
here. i hope this is ok with you, emil?
in a desperate attempt to delay packing until the last possible minute,
i'll comment on emil's comments now and make the code changes when i
come back.
Here is my review on r2llib-0.10
r2llib.h:1:
General Comments:
=================
I don't like too much #include guards that start with an underscore. As far as
I know, these identifiers are reserved by the standard C and C++ libraries, so
if you are out of luck, ugly conflicts might occur.
I prefer changing them from
#ifndef _FOO_H_
#define _FOO_H_
...
#endif /*_FOO_H_*/
to:
#ifndef FOO_H_ /* or FOO_H whichever you prefer */
#define FOO_H_
...
#endif /*FOO_H_*/
r2llib.h: 44:
r2llib.h:
=========
Since this is a public header file
1) I would make all the functions defined here start with the r2l prefix.
That means
Replace
enable_r2l() with r2l_enable(), etc
get_current_r2l_state() with r2l_get_current_state(), etc
set_biditext_base_state_neutral() with r2l_set_biditext_base_state_neutral(),
etc
In this way, you will prevent global namespace cluttering and avoid possible
conflicts with other header files.
mulix: agreed. this was on my internal TODO for quite some time, but i
didnt want to make interface change during the development cycle.
r2llib.h: 1:
2) As a courtesy to potential C++ programs which might use your code (refeshd
is one of them, hinthint :-) you should brace the r2llib header file like this:
#ifndef R2LLIB_H_
#define R2LLIB_H_
#ifdef __cplusplus
extern "C" {
#endif
/* contents of the header file goes here */
#ifdef __cplusplus
}
#endif
#endif /* R2LLIB_H_ */
You need not do this for all header files, only for the public header file(s).
mulix: agreed.
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 as expected
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 :)
r2llib.c:51:
Line 51: Like my last coment above regarding the case when malloc() fails.
r2llib.c:53:
Line 53-55: memcpy(tmpfname, prefix, strlen(prefix)+1); traverses prefix
twice, once for strlen() and once for the actual copying. I think that
strcpy(tmpfname, prefix); is better. I also would have used:
sprintf(tmpfname,"%s/%s",prefix,fname); instead of the memcpy()/strncat()
combination.
mulix: agreed. even though it's not a critical path, i was lazy here. i
will fix this when i get back.
r2llib.c:60:
Line 60: I would place an assert(fname != NULL); bomb before this line.
mulix: no need, fname *cannot* be NULL here, as we've set it to
DEFAULT_FILE_NAME if it was null at line 39.
r2llib.c:162:
In function r2l_init_from_text_serialization()
----------------------------------------------
I would place
assert(repr != NULL);
at the start of it. See also the following comments.
mulix: agreed.
r2llib.c:71:
For the rest of the functions
-----------------------------
I would have placed an
assert(rtl != NULL);
at the start of each of them. If compiled in debug mode this will cause the
user code to crash. And this is what he/she deserves if he/she passes an
r2llib_t token that has *not* been obtained from a *successful* call to
r2l_init(). Otherwise I would try to handle the case if r2l is NULL like
you did in your code (with the exception of r2ldestroy()).
mulix: hmm, the case where rtl is null will be handled by
r2llib_data_get_file_name(), but for better debuggability you are right.
(assert allows you to pinpoint failure locations, since the function
innermost at the call stack is the one where the error occured).
r2llib_data.h:1:
File r2llib_data.h
==================
Change the include guard to something like:
R2LLIB_DATA_H_
r2llib_data.c:20:
File r2llib_data.c
==================
In function create_r2llib_data()
--------------------------------
Line 20: I would prefer the function to fail if file_name == NULL. Otherwise
the code will fail in various weird ways because an empty string is not
a legal filename.
mulix: hmmm. we need to agree when it is ok to fail. i suspect that you
are right here and i was too permissive in my code - no point in going
on with a null fname (or null rtl, for that mater).
r2llib_data.c:34:
In function destroy_r2llib_data()
Line 34: You don't check if rtl is NULL. I know the behavior is undefined
so the user can be safely blamed for crashing here. But in all other cases
you try not to crash. So I think that then you should try to be consistent.
mulix: you are right, we should definitely check for (rtl != NULL)
here.
r2l_state.h:1:
File r2l_state.h:
=================
No comments.
r2l_state.c:51:
File r2l_state.c
================
In function toggle_r2l_state()
------------------------------
Line 51: In general it is a good idea to place a default: label in every switch
statement. Will it ever be reached in this case? No unless there are bugs
in get_r2l_state() causing it to return garbage. So the default: clause
should have something like:
default:
assert(0*state);
state = R2L_ERROR;
break;
mulix: agreed.
parse_argv.h:1:
File parse_argv.h
=================
No comments.
parse_argv.c:29:
File parse_argv.c
=================
In function parse_command_line()
--------------------------------
I would make this function check and delete the --file-name argument only
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 bit biased to the case
r2ltest == biditext.
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.
file_ops.c:77:
In function create_file()
Line 77: The if statement is superfluous. But maybe it helps you with the
debugging (setting a breakpoint on return -1??)
mulix: you are right, and this is a very good catch, as create_file() is
in the fast path! will be fixed.
These functions are not currently used, but if you decide some sunny day
(or some moonlight night) to use them:
file_ops.c:123:
In function write_file():
-------------------------
Line 123: I think that the result of close() should be checked here. failing
close() might indicate some ugly things such as a full disk.
mulix: agreed.
file_ops.c:144:
In function read_file():
------------------------
Line 144: The cast to void* is unnecessary.
mulix: i prefer explicit to implicit conversion.
biditext_base_state.h:1:
File biditext_base_state.h
==========================
No comments.
biditext_base_state.c:35:
File biditext_base_state.c
==========================
In function set_biditext_base_state()
-------------------------------------
Line 35: I would not make the default: and BT_BASE_NEUTRAL: the same.
The former means that there is a bug in the caller to
set_biditext_base_state() so I would assert() or at least fail,
while the latter is OK.
For some reason I don't like case labels surrounded by prantheses, but this is
religious.
mulix: hmm. i dont know. i want to be as robust as possible, even if the
user is doing stupid things.
biditext_base_state.c:42:
Line 42: I think that close() should be checked for the same reasons like above
(write_file() in file_ops.c)
mulix: unlike write_file(), what would we do here if close() failed? we
cannot set the state to error, and this is the only error reporting
mechanism we have... ergo, no point in checking close().
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.
main.c:60:
File main.c
===========
Even if it is just a test program, *never*, but I mean *never* place any
active code inside assert()!! assert() won't do anything if the program
will be compiled with NDEBUG macro defined.
mulix: debateable, since i dont see any use for a *test program* without
debugging enabled...
mulix: again, thank you very much for taking the time to review the
source!
--
mulix
http://www.advogato.com/person/mulix
linux/reboot.h: #define LINUX_REBOOT_MAGIC1 0xfee1dead