Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

There's always a question of what 'improve' might mean. I don't see anything here that is 'wrong,' so mostly I'm commenting on 'style.' That said, if I were doing a code review, these are some of the things I'd mention:

  • You have lots of very clear comments. That's a good thing, especially in assembler. I can't count the number of times I've read code and thought "What the heck was this guy THINKING?" This can be especially important when the code is wrong: He thinks it's doing x, but the code actually does y, so I am probably safe fixing it.
  • You don't have a comment that describes what the program does. While your SE post has a (misspelled) description, the code does not.
  • While it is not an issue for such a short program, you might want to start thinking in terms of 'routines.' Having a chunk of code that counts the length of a string (or that prints a string) that you can copy into your next program can be useful.
  • While it works, I wouldn't get into the habit of using repne. It's not the best performing way to do this kind of loop (see http://stackoverflow.com/a/34141835/2189500https://stackoverflow.com/a/34141835/2189500). BTW, this is one of the reasons to use a high level language (like C). Compilers already know all this stuff and (try to) use the best possible construct.
  • I expect that with a bit of fiddling (by using an offset on the mov) you can remove the dec edi/inc edx.
  • I might consider changing the jb ExitFailure. jb can only jump over a certain amount of code, so your existing construct will work even if PopEnv was far away. But since your code is small, you could do:

a

jnb PopEnv              ; get another env

ExitFailure:
mov eax,1               ; sys_exit
mov ebx,-1              ; EXIT_FAILURE
int 80h                 ; make the call

Exit:
mov eax,1               ; sys_exit
mov ebx,0               ; EXIT_SUCCESS
int 80h                 ; make the call

In summary, my comments are mostly just piddly 'style' stuff.

There's always a question of what 'improve' might mean. I don't see anything here that is 'wrong,' so mostly I'm commenting on 'style.' That said, if I were doing a code review, these are some of the things I'd mention:

  • You have lots of very clear comments. That's a good thing, especially in assembler. I can't count the number of times I've read code and thought "What the heck was this guy THINKING?" This can be especially important when the code is wrong: He thinks it's doing x, but the code actually does y, so I am probably safe fixing it.
  • You don't have a comment that describes what the program does. While your SE post has a (misspelled) description, the code does not.
  • While it is not an issue for such a short program, you might want to start thinking in terms of 'routines.' Having a chunk of code that counts the length of a string (or that prints a string) that you can copy into your next program can be useful.
  • While it works, I wouldn't get into the habit of using repne. It's not the best performing way to do this kind of loop (see http://stackoverflow.com/a/34141835/2189500). BTW, this is one of the reasons to use a high level language (like C). Compilers already know all this stuff and (try to) use the best possible construct.
  • I expect that with a bit of fiddling (by using an offset on the mov) you can remove the dec edi/inc edx.
  • I might consider changing the jb ExitFailure. jb can only jump over a certain amount of code, so your existing construct will work even if PopEnv was far away. But since your code is small, you could do:

a

jnb PopEnv              ; get another env

ExitFailure:
mov eax,1               ; sys_exit
mov ebx,-1              ; EXIT_FAILURE
int 80h                 ; make the call

Exit:
mov eax,1               ; sys_exit
mov ebx,0               ; EXIT_SUCCESS
int 80h                 ; make the call

In summary, my comments are mostly just piddly 'style' stuff.

There's always a question of what 'improve' might mean. I don't see anything here that is 'wrong,' so mostly I'm commenting on 'style.' That said, if I were doing a code review, these are some of the things I'd mention:

  • You have lots of very clear comments. That's a good thing, especially in assembler. I can't count the number of times I've read code and thought "What the heck was this guy THINKING?" This can be especially important when the code is wrong: He thinks it's doing x, but the code actually does y, so I am probably safe fixing it.
  • You don't have a comment that describes what the program does. While your SE post has a (misspelled) description, the code does not.
  • While it is not an issue for such a short program, you might want to start thinking in terms of 'routines.' Having a chunk of code that counts the length of a string (or that prints a string) that you can copy into your next program can be useful.
  • While it works, I wouldn't get into the habit of using repne. It's not the best performing way to do this kind of loop (see https://stackoverflow.com/a/34141835/2189500). BTW, this is one of the reasons to use a high level language (like C). Compilers already know all this stuff and (try to) use the best possible construct.
  • I expect that with a bit of fiddling (by using an offset on the mov) you can remove the dec edi/inc edx.
  • I might consider changing the jb ExitFailure. jb can only jump over a certain amount of code, so your existing construct will work even if PopEnv was far away. But since your code is small, you could do:

a

jnb PopEnv              ; get another env

ExitFailure:
mov eax,1               ; sys_exit
mov ebx,-1              ; EXIT_FAILURE
int 80h                 ; make the call

Exit:
mov eax,1               ; sys_exit
mov ebx,0               ; EXIT_SUCCESS
int 80h                 ; make the call

In summary, my comments are mostly just piddly 'style' stuff.

Source Link
David Wohlferd
  • 1.5k
  • 1
  • 8
  • 17

There's always a question of what 'improve' might mean. I don't see anything here that is 'wrong,' so mostly I'm commenting on 'style.' That said, if I were doing a code review, these are some of the things I'd mention:

  • You have lots of very clear comments. That's a good thing, especially in assembler. I can't count the number of times I've read code and thought "What the heck was this guy THINKING?" This can be especially important when the code is wrong: He thinks it's doing x, but the code actually does y, so I am probably safe fixing it.
  • You don't have a comment that describes what the program does. While your SE post has a (misspelled) description, the code does not.
  • While it is not an issue for such a short program, you might want to start thinking in terms of 'routines.' Having a chunk of code that counts the length of a string (or that prints a string) that you can copy into your next program can be useful.
  • While it works, I wouldn't get into the habit of using repne. It's not the best performing way to do this kind of loop (see http://stackoverflow.com/a/34141835/2189500). BTW, this is one of the reasons to use a high level language (like C). Compilers already know all this stuff and (try to) use the best possible construct.
  • I expect that with a bit of fiddling (by using an offset on the mov) you can remove the dec edi/inc edx.
  • I might consider changing the jb ExitFailure. jb can only jump over a certain amount of code, so your existing construct will work even if PopEnv was far away. But since your code is small, you could do:

a

jnb PopEnv              ; get another env

ExitFailure:
mov eax,1               ; sys_exit
mov ebx,-1              ; EXIT_FAILURE
int 80h                 ; make the call

Exit:
mov eax,1               ; sys_exit
mov ebx,0               ; EXIT_SUCCESS
int 80h                 ; make the call

In summary, my comments are mostly just piddly 'style' stuff.