Re: [opensuse] Assembly Language program

On Saturday 02 June 2007 04:22, azeem ahmad wrote:
hi list
i am about to make a bootable floppy for test
but i am being unable to get it done
please review the code below and tell me if there is any problem with it

Very funny!!! I have recently left a job in part because I became very tired
of reviewing rubbish code in assembler when I felt certain I could write the
same thing in C and leave less problems if my C was unreviewed than there
would be if I reviewed someone else's misguided assembler.

---------------------------------------------------------------------------
------------------------------------------------------- .code16 #assembler
directive to start 16-bit real mode for execution .text /*assembler
directive to tell the start of 'read-only'
code segment*/
.org 0x00 /*assembler directive to set the origon to sector 0
needed to copy the program to the very first sector
of the floppy disk*/

.global _start /*assembler directive to export the start section to
all other programs, i.e. to make it visible to programs
like linker or other user programs*/

... (etc)

Good grief man, no one in their right mind is going to review that. Or if they
do, you should doubt the value of their comments.

If anyone asked me to review something looking like that, I would have told
them to represent it, properly formatted. I used to teach our company's code
review course, and I said that some code was so poor that it did not meet the
entry criteria for review, and that it should be returned without review.
Yours is so badly formatted thatnobody should waste their time reviewing it.

The comment I make is harsh - but please understand that if you want someone
to even look at your code for 5 minutes, you have to convince them that you
made good judgements when you worked on it for 5 hours.

Try something like this below - and don't insult your reviewer by telling him
that each line beginning with '.' is an assembler directive. If he knows this
already, you are making it harder for him to read. If he doesn't know this,
you are wasting his time and yours by having him review it.

.code16 # 16-bit real mode for execution
.text # start of 'read-only' code segment
.org 0x00 # origin at sector 0
# - ie floppy disk sector 0

.global _start # make _start visible for linking

More:

call _disp # call subroutine disp
Useless comment. Says what you can see from the code

_disp: #label of the disp routine
That too is a useless comment. Again, either we know it's a label, or we have
nothing useful to contribute to a review. No comment is needed here, it just
makes the rest harder to read.

lodsb # load the string pointed by ds:si in a
# - byte by byte manner into the accumulator
This is a misleading comment. You should be shot for this. AFAICS, this
instruction loads just 1 byte, it does not do anything byte by byte - it is
the _disp loop as a whole which does byte by byte.

jz ret # jump if al zero to return
Another useless comment for the same reason. I am not hot on Intel assembler,
so I don't know it is al which is zero compared - but I do know that to
review this I would have to know what jz does - so as reviewer, I know it is
my responsibility either to understand each line or to find out what it does

or %al, %al # check if the entire string has been loaded byte-wise
# by oring al to al,
# if the result is 0, there are no more bytes to load
This is a far more useful comment, because it reveals strategy - ie what you
are trying to do. Line 2 of the comment [by oring al to al] is useless
because it says how you are trying to do it, but we can already see that from
the code. You should tell your reviewer what you are trying to do - he is not
a mind reader, otherwise you are wasting his time trying to work it out.

Overall, taking the _disp loop as an example, you need a good 'strategy'
comment to say what the loop is intended to achieve. If this comment is good,
you need less comments on the individual lines.


If your intention is to make a bootable floppy, you may find this link useful:
http://linuxgazette.net/issue84/dashti.html. Good luck with your coding - but
even if you are fixing the comments in this code, you've had your 5 minutes
from me. May be this is not the help you expected - I have been been hard on
your style - but I hope that it will help you in the future.

--
To unsubscribe, e-mail: opensuse+unsubscribe@xxxxxxxxxxxx
For additional commands, e-mail: opensuse+help@xxxxxxxxxxxx

Relevant Pages

  • Re: [opensuse] Assembly Language program
    ... would be if I reviewed someone else's misguided assembler. ... nothing useful to contribute to a review. ... If your intention is to make a bootable floppy, you may find this link useful: ... intel assembler in useful start point, ...
    (SuSE)
  • Re: -mm -> 2.6.13 merge status
    ... > I would in particular love to have you Andi Kleen do a full review of V4 ... This belongs to include/linux, not reiser4. ... Please drop the useless parameter comments. ... current subsystem specific allocators out, so do not introduce a new one. ...
    (Linux-Kernel)
  • Re: Discontinued Certification
    ... >an Assembler program maintained, ... related to 'our' business. ... management, the project simply should never get that far. ... review, review and review.... ...
    (microsoft.public.cert.exam.mcsd)
  • Re: META: The Problem of Subjectivity
    ... are also completely useless in and of themselves. ... You've said two things in various posts that directly contradict each ... Although it does help if the reviewer makes it clear that the review ... identifies mistakes that the author made (including spelling mistakes, ...
    (rec.arts.comics.creative)
  • Re: Outdoor Ashtray Recommendations?
    ... IOW, pretty useless for outdoors. ... Lots of good cigar info, the ASC Birthday page, FAQs, vendors and more at ... A "great" review is one with the name of the cigar before the review text in the body of the post. ...
    (alt.smokers.cigars)