1
\$\begingroup\$

Building upon my previous question, I have come up with a method that'll search through a buffer and return a pointer to the end of the line. The primary purpose of the function to extract rows from a csv file using the logic specified in RFC4180; the third parameter isQuotedSequence can be used in cases where parsing is parallelized or started in the middle of a string that is known to be quoted.

Notes:

  • the excessive amount of comments are for people who may not know the language but still want to try and understand what the code is doing
  • registers are renamed primarily to aid in refactoring but also enable some rudimentary static analysis to be performed
  • the isQuotedSequence variable is placed in the rdx register in order to mimic operations like mul that return a 128-bit result; I'm not sure if this is actually usable by anything other than ASM or supported by the Windows x64 or System V ABIs though...


Anything info that could be used to make the function safer or more performant would be most useful; thanks for your review!


Code:

COMMENT @
    C Interface:
        extern char* ReadLine(const char* bufferOffset, const char* bufferTail, long long isQuotedSequence);

    Reference:
        https://tools.ietf.org/html/rfc4180
@

;-----------------------------; (CONSTANTS)
CARRIAGE_RETURN  = 00000000Dh
DOUBLE_QUOTE     = 000000022h
LINE_FEED        = 00000000Ah
TRUE             = 000000001h
;-----------------------------; (ARGUMENTS)
arg0             textequ <rcx>
arg1             textequ <rdx>
arg2             textequ <r8>
;-----------------------------; (LOCALS)
bufferOffset     textequ <rax>
bufferTail       textequ <r9>
currentCharacter textequ <ecx>
isQuotedSequence textequ <rdx>
nextCharacter    textequ <r8d>

.code

ReadLine proc
    mov bufferOffset, arg0                         ; initialize [bufferOffset]
    mov bufferTail, arg1                           ; initialize [bufferTail]
    mov isQuotedSequence, arg2                     ; initialize [isQuotedSequence]
    cmp bufferOffset, bufferTail                   ; validate that there are more characters to read
    jge ReadLine@Return                            ; if end of file reached, jump to Return
    movzx nextCharacter, byte ptr[bufferOffset]    ; extract [nextCharacter] from [bufferOffset]
ReadLine@NextChar:
    mov currentCharacter, nextCharacter            ; shift [nextCharacter] into [currentCharacter]
    add bufferOffset, 1h                           ; increment [bufferOffset]
    cmp bufferOffset, bufferTail                   ; validate that there are more characters to read
    jge ReadLine@Return                            ; if end of file reached, jump to Return
    movzx nextCharacter, byte ptr[bufferOffset]    ; extract [nextCharacter] from [bufferOffset]
    cmp currentCharacter, DOUBLE_QUOTE             ; compare [currentCharacter] to QUOTE_DOUBLE
    jz ReadLine@HitDoubleQuote                     ; if equal, jump to HitDoubleQuote
    cmp currentCharacter, CARRIAGE_RETURN          ; compare [currentCharacter] to CARRIAGE_RETURN
    jz ReadLine@HitCarriageReturn                  ; if equal, jump to HitCarriageReturn
    cmp currentCharacter, LINE_FEED                ; compare [currentCharacter] to LINE_FEED
    jz ReadLine@HitLineFeed                        ; if equal, jump to HitLineFeed
    jmp ReadLine@NextChar                          ; jump to NextChar
ReadLine@HitDoubleQuote:
    xor isQuotedSequence, TRUE                     ; invert [isQuotedSequence] indicator
    jmp ReadLine@NextChar                          ; jump to NextChar
ReadLine@HitCarriageReturn:
    cmp nextCharacter, LINE_FEED                   ; compare [nextCharacter] to LINE_FEED
    jz ReadLine@NextChar                           ; if equal, jump to NextChar
ReadLine@HitLineFeed:
    cmp isQuotedSequence, TRUE                     ; compare [isQuotedSequence] to TRUE
    jz ReadLine@NextChar                           ; if equal, jump to NextChar
ReadLine@Return:
    ret                                            ; return to caller
ReadLine endp
end
\$\endgroup\$
2
\$\begingroup\$

You're treating comparison of addresses as signed values, instead of unsigned. For example, after you execute cmp bufferOffset, bufferTail, you use the jge condition, which is jump greater or equal. This is intended for signed values. You should use jae jump above or equal instruction instead. This condition needs to be changed in several places.

Rather than add bufferOffset, 1h, just use inc bufferOffset.

From a readability perspective, having all those leading zeros on you constants makes the actual value a little harder to read, and implies that they could potentially have some value that would be that large. Since most of those values are character codes, two digits should suffice.

\$\endgroup\$
  • \$\begingroup\$ Regarding consistency with carriage return, this is to handle all possible cases of valid line endings: \r\n, \r, \n. In other words, the line feed after a carriage return is optional (thanks to varying OS behavior). \$\endgroup\$ – Kittoes0124 Mar 11 at 19:05
  • \$\begingroup\$ @Kittoes0124 I got confused by the nextCharacter check. I've removed that paragraph from the review. \$\endgroup\$ – 1201ProgramAlarm Mar 11 at 19:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.