|
PogoboyKramer wrote: I actually removed this one from a piece of code this week. And believe it or not, it was actually being called.
You fool! Now *nothing* will be valid. Wasn't it better when it was ALL valid? How is anything supposed to work unless it's valid?
He said, "Boy I'm just old and lonely,
But thank you for your concern,
Here's wishing you a Happy New Year."
I wished him one back in return.
|
|
|
|
|
|
So, yeah, spent a few days (at night, as this is not for my day job) looking for a bug in this code...yep, the bug? Needed to place the return command "ret" after white space, ie. not at the start of a line. Side-effect? It doesn't return, just keeps running the next instruction in memory after ret.
DisplaySelectList:
cls()
;========================================
;
; Input:
; AppSelectListControl.TopPosition
; AppSelectListControl.ListItemsIndirect
; AppSelectListControl.LineStartAddys
;
;
; Start reading the LineStartAddys
; at the TopPosition position
;
; Set the TextInverse if the value in
; ListItemsIndirect at position is equal to
; SelectedItem value.
;
; Determine if TopPosition ptr is the first
; line of a list item, if so, then print
; that number.
;
; Print starting at the LineStartAddys[x]
; until NULL reached.
ld a, (AppSelectListControl.TopPosition)
ld (AppSelectListControl.PositionCount), a ; save to variable
;.breakpoint check_a_for_top_position
nop
LoopPositions:
;=======================================
; Check that PositionCount - TopPosition
; is not equal to 9, else stop printing
;=======================================
ld a, (AppSelectListControl.TopPosition)
ld b, a
ld a, (AppSelectListControl.PositionCount)
sub b
cp 9
.breakpoint check_z_for_reached_9_a_vs_b
;==============Base case displayed == 9 ? then stop
jp z, donePrintingSelectList
;=============================================
; Get the addy of the string and save to stack
;
;
;=============================================
ld de, AppSelectListControl.LineStartAddys ;calculate offset of array start
.breakpoint de_has_LineStartAddy
;calculate offset using TopPosition
xor a
ld h, a
ld a, (AppSelectListControl.PositionCount)
ld l, a
add hl, hl ;double because array is of 2byte items
add hl, de ; offset calculated
;.breakpoint check_hl_for_addy_of_LineStartAddys_of_index
;==============
;-->ld (hl), hl
;==============
ld c, (hl)
inc hl
ld b, (hl)
push bc ; Save the addy to list item's string
; to be used by print string.
;==========================
; Get next position's addy,
; could be 0000
;
;==========================
inc hl
ld a, (hl)
ld (AppSelectListControl.NextAddy), a
inc hl
ld a, (hl)
ld (AppSelectListControl.NextAddy+1), a
;reset hl back to ptr to addy of position
dec hl
dec hl
dec hl
.breakpoint just_pushed_bc_as_addy_string
;;===========================
;; Read value of addy into hl
;;===========================
;ld a, (de)
;ld l, a
;inc de
;ld a, (de)
;ld h, a
;;.breakpoint check_hl_has_value_of_listItem_string
;================================================
; if addy is null, then stop printing select list
;================================================
ld a, b
or a
;.breakpoint check_z_for_NULL_listitem_addy1
jp nz, {+} ; don't quit
ld a, c
or a
;.breakpoint check_z_for_NULL_listitem_addy2
jp nz, {+} ; don't quit
pop bc
;.breakpoint check_z_for_1
jp donePrintingSelectList
+:
;=====>base case: TopPosition == 0, consider this
; line the start so skip to that section
ld a, (AppSelectListControl.PositionCount)
;.breakpoint a_has_position_count
ld b, a
xor a
or b
jp nz, NextPosition
xor a
push af ; push 0 as list item number
jp printFormattedNumber
; b has current Position (initially was TopPosition)
NextPosition:
;check the value at PositionCounter - 1 in ListItemsIndirect
dec b
;.breakpoint check_b_for_positionCounter_minus_1
ld de, AppSelectListControl.ListItemsIndirect
;calc offset using TopPosition
xor a
ld h, a
ld l, b ; b has TopPosition - 1
add hl, de
ex de, hl
;.breakpoint check_de_for_addy_of_position_minus_1
; de has the address of the TopPosition - 1 index of
; the ListItemsIndirect array
ld a, (de) ; ld last position's list item number
;.breakpoint check_a_for_lastPositions_listItem_number
push af ; save last list item number
inc de
ld a, (de) ; de is at Position now
ld (AppSelectListControl.ListItemPtr), a ; save for later checking highlight status
;.breakpoint check_listItemPtr_and_de
ld b, a ; ld Position's list item number
;.breakpoint check_b_for_listItem_number
pop af ; get last position's list item number
cp b ; are they the same?
;.breakpoint check_z_for_are_they_same
jp z, printString ; jump past code for making formatted number
push bc
printFormattedNumber:
;===============================================
; Print a formatted number based on list item on
; the stack. (upper-byte)
;===============================================
pop af ; get list item number
.breakpoint check_a_for_listCount1
inc a
;.breakpoint check_a_for_listCount2
;.breakpoint a_has_listItem_number
call Convert.AToDecimalText ; hl will point to decimal text w/ null
;.breakpoint check_hl_addy_has_formatted_number
;======================
; Print each character
;======================
res textInverse, (iy + textFlags) ; do not print highlight
-:
ld a, (hl)
; Check for NULL
xor b
cp b
jp z, {+}
ld a, (hl)
bcall(_VPutMap)
inc hl
jp {-}
+:
;.breakpoint printed_formatted_number
ld a, '.'
bcall(_VPutMap)
ld a, ' '
bcall(_VPutMap)
ld a, ' '
bcall(_VPutMap)
ld a, ' '
bcall(_VPutMap)
printString:
;==============================
; set the text inverse
; if listCount == selectedItem
;==============================
ld a, (AppSelectListControl.ListItemPtr)
ld b, a
ld a, (AppSelectListControl.SelectedItem)
;.breakpoint check_a_for_listcount_and_b_for_selectedItem
cp b
jp z, SetHighlight
res textInverse, (iy + textFlags)
jp doneHighlight
setHighlight:
nop
;.breakpoint Setting_highlight
set textInverse, (iy + textFlags)
doneHighlight:
;=================================
; Print the string starting at the
; addy found on the stack
;=================================
pop hl
;.breakpoint printString_start_hl_has_addy
nop
;========================================
;Get the next line break to know when to
; stop printing the string.
;========================================
-:
ld a, (hl)
xor b
cp b
jp z, {+} ; quit if null reached
;========
; quit if hl matches addy of next line break
ld a, (AppSelectListControl.NextAddy)
ld e, a
ld a, (AppSelectListControl.NextAddy+1)
ld d, a
;.breakpoint check_de_for_next_addy
ld a, l
cp e
jp nz, {++}
ld a, h
cp d
jp nz, {++}
jp {+}
++:
ld a, (hl)
;.breakpoint check_hl_as_chr_ptr
bcall(_VPutMap)
inc hl
jp {-}
+:
;.breakpoint done_printing_this_line
;==================
; NewLine operation
;==================
ld a, (penrow)
ld b, 7
add a, b
ld (penrow), a
xor a
ld (pencol), a
;========================
; Increment PositionCount
; then check if addy
;========================
inc_byte(AppSelectListControl.PositionCount)
;.breakpoint moved_to_next_position
jp LoopPositions
donePrintingSelectList:
.breakpoint done_printing_select_list
nop
ret < ------- NEEDS SPACE IN-FRONT OF IT TO WORK...WTF
|
|
|
|
|
I've seen a similar bug caused by a compiler not behaving correctly when the last line in a file doesn't end in a newline character.
Steve
|
|
|
|
|
Had that with early Visual C++ and .h files: no newline == massive fail by compiler.
Real men don't use instructions. They are only the manufacturers opinion on how to put the thing together.
|
|
|
|
|
Ah! You have brightened up a dull grey Thursday - it has been a few years since I looked at any Z80 code.
It's a bugger when you miss that an instruction is being treated as a label.
Real men don't use instructions. They are only the manufacturers opinion on how to put the thing together.
|
|
|
|
|
Its interesting to see how my suffering helps others.
|
|
|
|
|
Yes, things like that are really a pain to find.
You didn't have to post that much code to get the point across though.
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
jp nz, {++}
jp {+}
++:
I might be missing something, but why isn't that just
jr z, {+}
|
|
|
|
|
I think I'll take CIDiv's advice next time....you're probably not missing something...
|
|
|
|
|
That's NOT what she said!!!
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
So, today I came across this gem:
if (!(value > 0))
{
throw new ArgumentOutOfRangeException("....");
} Could we not do this?
if (value <= 0)
|
|
|
|
|
Depends on the language and implementation. Perhaps the operators are implemented differently (if they are overloaded). For example, sometimes it makes sense to compare two butterflies for equality, but you can't say one is greater than another. Or maybe only one operator was overloaded, but another wasn't (a poor practice in itself).
That being said, whoever wrote that was probably just being a doofus.
|
|
|
|
|
To put in context - it's a C# int .
|
|
|
|
|
When I see code like you quoted, I go looking for things like
unsigned short value;
just in case the comparison *might* be intentionally smart.
[OT, but you can see the thought process]
Java's lack of unsigned integer types is:
(a) a PITA
(b) a lifesaver
(c) both
(d) none of the above.
Discuss.
Software rusts. Simon Stephenson, ca 1994.
|
|
|
|
|
e) A symptom of scripting languages.
|
|
|
|
|
Unsignedness would not excuse the author (value == 0 ), floats may have (NaN would behave differently)
|
|
|
|
|
Pete O'Hanlon wrote: if (!(value > 0)){ throw new ArgumentOutOfRangeException("....");}
Could we not do this?
if (value <= 0)
There could be a perfectly reasonable explanation: The code maybe like the following previously
<br />
if (!isValid(value)){ throw new ArgumentOutOfRangeException("....");}<br />
Then someone decided to simplify it by replacing isValid(value) with (value>0).
|
|
|
|
|
Sadly not. I know the person who wrote this - he's not that big on any form of refactoring.
|
|
|
|
|
It might just be a case of lazy programming. I've seen code where if statements were initially written in the opposite state of what was intended and then instead of fixing the whole statement, the ! was simply thrown in front.
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
That is the most likely explanation.
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
I think this is the case also. Every once in a while (once in a blue moon), I do it too. Not that I'm lazy...I just had a logical bug, decided to see if the opposite would work, it did work, so I move to the next problem thinking I'll come back and fix it later.
|
|
|
|
|
Sounds like it was written by a lawyer.
|
|
|
|
|
It may be that the person is using a general pattern of testing whether required input conditions are not met by throwing on their inverse. When preconditions are more complicated, it can sometimes be cleaner say something like:
if (!(foo || bar))
throw new InvalidArgumentException("Neither foo nor bar was valid");
Than to say:
if (!foo && !bar)
throw new InvalidArgumentException("Neither foo nor bar was valid");
If one habitually writes pre-checks based upon preconditions, code will probably read better if one consistently uses the same style. If there will be any need to test preconditions using throw-if-not-met logic, it may be best to do so consistently.
Also, btw, if one is going to be pasting code into something like a web-based forum, it may be helpful to write conditions so as to avoid the less-than sign. My usual rewrite of a less-than-zero condition for web posting would typically be "0 > whatever" rather than "!(whatever >= 0)", but some people may prefer the latter style.
|
|
|
|
|
Of course you could use your form:
if (value <= 0)
But if values greater than zero are OK, and anything else should give an exception, then why not write it in the original form:
if (!(value > 0))
{
throw new ArgumentOutOfRangeException("....");
}
This could be considered a more direct translation of the requirement, and therefore more understandable.
Of course, it may just have been a lack of thought, but I'd argue there are times when it would be beneficial not to simplify expressions, particularly when you have a compiler that will reduce it to the same form for you so there is no performance penalty.
|
|
|
|
|