Re: Strings in Ada83

[mfb@mbunix.mitre.org (Michael F Brenner)]

Scott said: > ... strings package is at
            > http://www.csee.usf.edu/~dick/cstring.html 
            > ... your comments would be most welcome. Thanks... 

If you ever choose to revise it, you might consider these ideas:
   (1) Separation of I/O from computation. For example,
       The get_line and put_line could be put in a second package
       (or a child package in Ada-95). With suitable revisions,
       the package would then be a pure package which would be 
       subject to additional optimizations. In Ada-95 this pureness
       can be celebrated with a specific pragma, but an Ada-83
       optimizer can still do the data analysis to recoginize the
       pureness of the code. Making the string manipulation portion 
       of the package dependent upon text_io makes it non-portable. 
       Realtime embedded systems may not have a text_io
       connected, consider a satellite in orbit. In addition,
       text_io usually brings in a lot of software which is
       a memory hog. More important than the practical problems
       with forcing the user to use text_io in order to use
       strings is the lack of cohesion in the package caused by
       mixing I/O with string manipulation; to increase the 
       cohesion, let the package do one thing (string manipulation)
       without a very complex other thing (I/O).

   (2) Use of a limited type. Since you chose to make it a limited type,
       why not make INSERT and DELETE functions instead of procedures,
       taking advantage of a fully functional style?

   (3) Use of a limited private type. Consider making type cstrings
       a limited private type, and convert all the functions to
       procedures, except tostring, length, and the comparision
       operators, taking advantage of the safety of limited private
       thypes.

   (4) Letting the user choose the size of each string. You might wish
       to change the type definition to permit each string to have its
       own size. Arrays, however, would have to have all strings of the
       same maximum length.

   (5) The exception cstring_null which is raised when the text_io file
       is not open, does not inform the user what the real problem is.

   (6) Have you tested this using the full capability of text_io to
       process files with end-of-page markers in them?

   (7) The loop in the less-than function would be easier to read, by
       removing the boolean variable GO and experiencing the corresponding
       lowering in the depth of nesting. To do this, replace the
       statement IF LEFT... /= RIGHT ... with the statement
       EXIT WHEN LEFT /= RIGHT. In most compilers, doing this would
       result in faster code because the I:=I+1 would then be the
       innermost nesting level of the loop and thus be slightly
       better optimized. This would not matter so much in a machine
       with a deep instruction pipeline or where the program cache
       is not voided by jumps. 

   (8) The body of the function less-than-or-equal-to can be simplified
       by noting that IF CONDITION THEN RETURN TRUE ELSE RETURN FALSE
       is equivalent to RETURN CONDITION.

   (9) There are no protection against uninitialized cstrings (as you  
       and this code may go into long or infinite loops when used
       on uninitialized cstrings.

   (10) Returning a maximum sized Ada string (say 1000000 characters
        long) for a 1 character long cstring is not efficient and this
        should be documented for the user. Better, why not return an
        Ada string that is the same length as the cstring?

   (11) In CSTRINGTONUMBER, the expression 
        IF NOT (('0'<=X) and (x<='9')) THEN
        would be more readable if replaced with
        IF X NOT IN '0'..'9' THEN

   (12) The exception cstring_null in cstringtonumber does not 
        adequately inform the user that a minus sign, plus sign,
        underline, or decimal point was used as part of a number
        and this code prohibits those characters.

   (13) It might be worth testing why MY_REVERSE does not actually 
        reverse all cstrings.

   (14) The variable FLAG would be obviated by replacing
            flag:=True;
            WHILE FLAG LOOP
               ... code ...
               if CONDITION then
                  flag := FALSE;
               end if;
            end loop

        with the simpler loop
            loop
              ... code ...
              exit when CONDITION;
            end loop;

   (15) When back filling with ASCII null characters, some of the
        loops fill the cstring up to max_size and some fill it
        up to max_size+1. This is often the sign of a bug. In this
        case, using suitable checks for non-initialized cstrings,
        the appropriate thing to do is not to fill the cstrings at
        all, but just add a single ASCII null at then end of the
        meat of the string. 

   (16) The formal parameter INDEX could be renamed AFTER to indicate
        its function in the procedure INSERT.

   (17) What happens when inserting something at the beginning of 
        a cstring?

   (18) What happens when deleting max_size characters from a string?

   (19) Is it true that BM_scan only located strings correctly when
        the pattern string being looked for (SUB) does not contain
        repetitious strings withing itself? For example, will it
        fail if INJINKINL is searched using the pattern INKINL, because
        it notices that INKINL doesnot match INJINK (the first 6 letters)
        and bumps up the count by 6, next trying to match INKINL against
        INL which also fails?

   (20) The test programs in your sample drivers do not flesh out all
        the bugs. An IF statement is needed for each feature of the
        package. This should only appear inside this package if it
        done entirely with ASSERT statements and use no I/O.
            procedure ASSERT (condition: boolean) is
              Scott_made_an_error: exception;
            begin
              if not condition then
                raise Scott_made_an_error;
              end if;
            end assert;

   (21) The data integrity paragraph of the user documentation is not
        accurate, promising that cstrings can only be accessed
        through the operations defined in the package. In addition,
        a private type can be access through the equality operator,
        the assignment operator, and through non-initialized 
        elaboration.

   (22) The Notes on Design and Implementation are not adequate to 
        fix the bugs or to maintain this software. 

   (23) The above comments, notwithstanding, this is a good piece
        of work and, if you concentrate an equal amount of time 
        writing test programs as you do writing lines of code, 
        you will solve many problems during your long and happy
        career.