Opened 4 months ago

Closed 3 months ago

#8646 closed feature request (fixed)

Distinguish between update frames in rts/Printer.c

Reported by: Tarrasch Owned by:
Priority: lowest Milestone:
Component: Runtime System Version: 7.6.3
Keywords: Cc: simonmar
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Difficulty: Unknown
Test Case: Blocked By:
Blocking: Related Tickets:

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 4 months ago.
8646.2.patch (2.4 KB) - added by Tarrasch 3 months ago.

Download all attachments as: .zip

Change History (11)

Changed 4 months ago by Tarrasch

comment:1 Changed 4 months ago by Tarrasch

  • Status changed from new to patch

comment:2 Changed 4 months 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 4 months 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 4 months 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 3 months ago by Tarrasch

comment:5 Changed 3 months 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 3 months ago by Tarrasch

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

comment:7 Changed 3 months 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 3 months ago by simonmar

  • Owner thoughtpolice deleted
  • Status changed from patch to new

comment:9 Changed 3 months ago by thoughtpolice

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.