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: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
