Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#8646 closed feature request (fixed)

Distinguish between update frames in rts/Printer.c

Reported by: Tarrasch Owned by:
Priority: lowest Milestone: 7.8.1
Component: Runtime System Version: 7.6.3
Keywords: Cc: simonmar
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

When doing printf-debugging and using Printer.c, it would be nice to see exact kind of update frame. (See attached patch)

Attachments (2)

8646.patch (2.3 KB) - added by Tarrasch 5 years ago.
8646.2.patch (2.4 KB) - added by Tarrasch 5 years ago.

Download all attachments as: .zip

Change History (12)

Changed 5 years ago by Tarrasch

Attachment: 8646.patch added

comment:1 Changed 5 years ago by Tarrasch

Status: newpatch

comment:2 Changed 5 years ago by ezyang

It might be more robust if, in the failure check, it re-checked the closure type, and reported "unknown update frame" or "not an update frame" instead.

comment:3 Changed 5 years ago by Tarrasch

Thanks for the quick reply, but I'm not sure I understood your suggestions:

  1. By re-check, do you mean to check that info->type == UPDATE_FRAME?
  2. By report, do you mean to use barf()?

Won't it be enough to just do (2.)?

comment:4 Changed 5 years ago by ezyang

Your function has an invariant, "must be an UPDATE_FRAME". So logically, it should check if that invariant is violated. I think your check in (1) is the right one but I haven't double-checked.

If an invariant is violated, barfing seems to be the right thing to do. Honestly, I would also barf when you expected an update frame but didn't get one.

Changed 5 years ago by Tarrasch

Attachment: 8646.2.patch added

comment:5 Changed 5 years ago by Tarrasch

I added a slightly changed patch. Now it barfs rather than returning a descriptive string-value. I think doing something as in (1) is redundant, since I exhaustively check against all types of update_frames in the if-statements. If it goes to the last else-branch, it's not an update frame.

comment:6 Changed 5 years ago by Tarrasch

Hi again, anything I can do to help this getting merged?

comment:7 Changed 5 years ago by simonmar

Owner: changed from simonmar to thoughtpolice

Ok, approved. I think I'll change the output slightly once it's in, but we can push it in for now.

comment:8 Changed 5 years ago by simonmar

Owner: thoughtpolice deleted
Status: patchnew

comment:9 Changed 5 years ago by thoughtpolice

Resolution: fixed
Status: newclosed

comment:10 Changed 5 years ago by hvr

Milestone: 7.8.1
Note: See TracTickets for help on using tickets.