Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/libhwbase/+/37530 )
Change subject: [WIP] Printf like debugging ......................................................................
Patch Set 1:
(8 comments)
I like SPARK ;) this is so much better to read than what I would have expected from a C implementation. Beside the indentation of course, we need to work on your editor configuration :)
https://review.coreboot.org/c/libhwbase/+/37530/1/debug/hw-print.adb File debug/hw-print.adb:
https://review.coreboot.org/c/libhwbase/+/37530/1/debug/hw-print.adb@47 PS1, Line 47: Parse_And_Put This is rather Put_And_Parse :)
https://review.coreboot.org/c/libhwbase/+/37530/1/debug/hw-print.adb@82 PS1, Line 82: Put (Format_String (Pos .. Pos)); Slight optimization: Currently this puts chars one by one. If we'd keep the start and end of the non-format string, we could put it all at once.
e.g. on top
is Non_Format_Start : constant Integer := Pos; Non_Format_End : Integer; ... begin
then here
when '%' => Non_Format_End := Pos;
and in the end
if Arg.Conversion = Arg_Invalid then Put (Format_String); elsif Non_Format_End > Integer'First then Put (Format_String (Non_Format_Start .. Non_Format_End - 1)); end if;
https://review.coreboot.org/c/libhwbase/+/37530/1/debug/hw-print.adb@118 PS1, Line 118: State := State_Arg_Modifier_Length; Alternatively, we could skip the `Modifier_Length` substate and use the state of `Arg.Length` instead, e.g.
when State_Arg_Modifier => case Format_String (Pos) is when 'l' => case Arg.Length is when Arg_Length_Normal => Arg.Length := Arg_Length_Long; when Arg_Length_Long => Arg.Length := Arg_Length_Long_Long; when others => Pos := Pos - 1; State := State_Arg_Conversion; end case; when others => Pos := Pos - 1; State := State_Arg_Conversion; end case;
https://review.coreboot.org/c/libhwbase/+/37530/1/debug/hw-print.adb@130 PS1, Line 130: State := State_Arg_Modifier; Nit, we already know that `State_Arg_Modifier` will fall through to `State_Arg_Conversion`. We can as well go directly to the latter.
https://review.coreboot.org/c/libhwbase/+/37530/1/debug/hw-print.adb@162 PS1, Line 162: type Int_Format is (Base_10, Base_16); This enumeration type doesn't seem too useful. Just using
subtype Int_Format is Positive range 2 .. 16;
would make the code easier to read (less conversions, but still type safe).
https://review.coreboot.org/c/libhwbase/+/37530/1/debug/hw-print.adb@162 PS1, Line 162: Int_Format Maybe `Int_Base` instead?
https://review.coreboot.org/c/libhwbase/+/37530/1/debug/hw-print.adb@166 PS1, Line 166: String_Begin Maybe just name it `Pos`. Less clear for the caller, but would make the code in here easier to read.
https://review.coreboot.org/c/libhwbase/+/37530/1/debug/hw-print.adb@207 PS1, Line 207: pragma loop_invariant (Item_C < Item_B ** String_Begin); Maybe we have to make it explicit that `Item_B` doesn't change in the loop. I'll give that a shot.