A finding about the bindings file is not a reason to stop a build
check_constants makes two kinds of finding and they were treated alike. A value that does not match, or a C name the header does not have, is the library contradicting the package and stops a build the way a permuted defstruct does. An enum nobody mapped and a rule that reaches nothing are about the package's own bindings file -- real, and worth fixing, but telling a lane that added a defenum to go and edit a config in a message shaped like "your layout is wrong" is the wrong thing to fail a build with. Those gate generate-c, where that file is edited. Also: a const prefix now counts as reaching a name before an explicit constant line is consulted, so a rule whose every match is also spelled out by hand is not reported as matching nothing.
This commit is contained in:
parent
9223c9002a
commit
4ff3e9a922
9
BUILT.md
9
BUILT.md
@ -392,6 +392,15 @@ next lane adds an enum, adds no line, and nothing notices. `enum Foo -` is how
|
|||||||
a package says out loud that the header has nothing to check `Foo` against —
|
a package says out loud that the header has nothing to check `Foo` against —
|
||||||
a sentence somebody wrote rather than a line nobody did.
|
a sentence somebody wrote rather than a line nobody did.
|
||||||
|
|
||||||
|
**The two kinds of finding have different dispositions, which is the one thing
|
||||||
|
worth getting right here.** A value that does not match, or a C name the header
|
||||||
|
does not have, is the *library* contradicting the package — the same kind of
|
||||||
|
thing a permuted `defstruct` is, and an ordinary build stops on it. An enum
|
||||||
|
nobody mapped and a rule that reaches nothing are about the package's own
|
||||||
|
`bindings` file: real, worth fixing, and not a reason to fail somebody's build
|
||||||
|
with a message shaped like "your layout is wrong". Those gate `flan generate-c`
|
||||||
|
instead, which is where that file is edited and where the author is standing.
|
||||||
|
|
||||||
`defconst` is deliberately not held to that. A package's constants are mostly
|
`defconst` is deliberately not held to that. A package's constants are mostly
|
||||||
its own — raylib's 26 colours, an example's screen size — and demanding a line
|
its own — raylib's 26 colours, an example's screen size — and demanding a line
|
||||||
for each would be noise with no second author behind it. `gesture-all` is the
|
for each would be noise with no second author behind it. `gesture-all` is the
|
||||||
|
|||||||
24
bin/main.ml
24
bin/main.ml
@ -299,7 +299,10 @@ let () =
|
|||||||
Printf.printf ";; every defenum member agrees with the header\n"
|
Printf.printf ";; every defenum member agrees with the header\n"
|
||||||
| bad ->
|
| bad ->
|
||||||
List.iter
|
List.iter
|
||||||
(fun (n, why) -> Printf.printf ";; DISAGREES %s: %s\n" n why)
|
(fun (x : Flan.Cimport.const_diff) ->
|
||||||
|
Printf.printf ";; %s %s: %s\n"
|
||||||
|
(if x.Flan.Cimport.cmapping then "UNMAPPED" else "DISAGREES")
|
||||||
|
x.Flan.Cimport.cname x.Flan.Cimport.cwhy)
|
||||||
bad))
|
bad))
|
||||||
|
|
||||||
(* Regeneration. [import-c] prints what it would produce; this writes it, and
|
(* Regeneration. [import-c] prints what it would produce; this writes it, and
|
||||||
@ -356,7 +359,10 @@ let () =
|
|||||||
x.Flan.Cimport.dflan x.Flan.Cimport.dsym x.Flan.Cimport.dwhy)
|
x.Flan.Cimport.dflan x.Flan.Cimport.dsym x.Flan.Cimport.dwhy)
|
||||||
r.Flan.Cimport.gsigs;
|
r.Flan.Cimport.gsigs;
|
||||||
List.iter
|
List.iter
|
||||||
(fun (n, why) -> Printf.eprintf "DISAGREES %s: %s\n" n why)
|
(fun (x : Flan.Cimport.const_diff) ->
|
||||||
|
Printf.eprintf "%s %s: %s\n"
|
||||||
|
(if x.Flan.Cimport.cmapping then "UNMAPPED" else "DISAGREES")
|
||||||
|
x.Flan.Cimport.cname x.Flan.Cimport.cwhy)
|
||||||
r.Flan.Cimport.gconsts;
|
r.Flan.Cimport.gconsts;
|
||||||
if r.Flan.Cimport.gwrote then
|
if r.Flan.Cimport.gwrote then
|
||||||
Printf.printf
|
Printf.printf
|
||||||
@ -367,14 +373,20 @@ let () =
|
|||||||
(List.length r.Flan.Cimport.ghidden) r.Flan.Cimport.gfns h
|
(List.length r.Flan.Cimport.ghidden) r.Flan.Cimport.gfns h
|
||||||
else begin
|
else begin
|
||||||
Printf.eprintf
|
Printf.eprintf
|
||||||
"flan generate-c: %s disagrees with %s — %d struct layouts, %d \
|
"flan generate-c: %s does not agree with %s — %d struct layouts, \
|
||||||
hand-written signatures and %d constants. Nothing was written: a \
|
%d hand-written signatures and %d constants, of which %d are a \
|
||||||
|
mapping the package has not declared. Nothing was written: a \
|
||||||
generated file made against a header the library does not match \
|
generated file made against a header the library does not match \
|
||||||
is the silent failure this check exists to prevent.\n"
|
is the silent failure this check exists to prevent, and a \
|
||||||
|
constant nothing is mapped to is one nothing checks.\n"
|
||||||
dir h
|
dir h
|
||||||
(List.length r.Flan.Cimport.gstructs)
|
(List.length r.Flan.Cimport.gstructs)
|
||||||
(List.length r.Flan.Cimport.gsigs)
|
(List.length r.Flan.Cimport.gsigs)
|
||||||
(List.length r.Flan.Cimport.gconsts);
|
(List.length r.Flan.Cimport.gconsts)
|
||||||
|
(List.length
|
||||||
|
(List.filter
|
||||||
|
(fun (x : Flan.Cimport.const_diff) -> x.Flan.Cimport.cmapping)
|
||||||
|
r.Flan.Cimport.gconsts));
|
||||||
exit 1
|
exit 1
|
||||||
end)
|
end)
|
||||||
|
|
||||||
|
|||||||
@ -955,13 +955,33 @@ let check_structs ~env ~(structs : (string * Ast.field list) list) (d : dump) =
|
|||||||
[gesture-all] is the honest example of one that is not: 1023 is the OR of
|
[gesture-all] is the honest example of one that is not: 1023 is the OR of
|
||||||
ten members and no C enumerator has that value to compare against. *)
|
ten members and no C enumerator has that value to compare against. *)
|
||||||
|
|
||||||
|
(* Two kinds of finding, and they have different dispositions.
|
||||||
|
|
||||||
|
[cmapping = false] is a disagreement with the *library*: a value that does
|
||||||
|
not match, or a C name the header does not have. That is the same kind of
|
||||||
|
thing a permuted [defstruct] is, and an ordinary build stops on it.
|
||||||
|
|
||||||
|
[cmapping = true] is about the package's own [bindings] file — an enum
|
||||||
|
nobody mapped, a rule that reaches nothing. Real, and worth fixing, but it
|
||||||
|
is not the library contradicting anybody, and stopping a build over it
|
||||||
|
would tell a lane that added a [defenum] to go and edit a config file in a
|
||||||
|
message shaped like "your layout is wrong". Those gate [generate-c], which
|
||||||
|
is where the config is being edited and where the author is standing. *)
|
||||||
|
type const_diff = { cname : string; cwhy : string; cmapping : bool }
|
||||||
|
|
||||||
let check_constants ~config
|
let check_constants ~config
|
||||||
~(enums : (string * (string * int64) list) list)
|
~(enums : (string * (string * int64) list) list)
|
||||||
~(consts : (string * Ast.expr) list) (d : dump) : (string * string) list =
|
~(consts : (string * Ast.expr) list) (d : dump) : const_diff list =
|
||||||
let found = Hashtbl.create 512 in
|
let found = Hashtbl.create 512 in
|
||||||
List.iter (fun (n, v) -> Hashtbl.replace found n v) d.consts;
|
List.iter (fun (n, v) -> Hashtbl.replace found n v) d.consts;
|
||||||
let out = ref [] in
|
let out = ref [] in
|
||||||
let say name fmt = Printf.ksprintf (fun m -> out := (name, m) :: !out) fmt in
|
let emit mapping name fmt =
|
||||||
|
Printf.ksprintf
|
||||||
|
(fun m -> out := { cname = name; cwhy = m; cmapping = mapping } :: !out)
|
||||||
|
fmt
|
||||||
|
in
|
||||||
|
let say name fmt = emit false name fmt in
|
||||||
|
let mapping_say name fmt = emit true name fmt in
|
||||||
(* One Flan name, its value, and the C name it claims to be. *)
|
(* One Flan name, its value, and the C name it claims to be. *)
|
||||||
let compare_one flan v cname =
|
let compare_one flan v cname =
|
||||||
match Hashtbl.find_opt found cname with
|
match Hashtbl.find_opt found cname with
|
||||||
@ -981,7 +1001,7 @@ let check_constants ~config
|
|||||||
(fun (ename, members) ->
|
(fun (ename, members) ->
|
||||||
match List.assoc_opt ename config.enum_prefixes with
|
match List.assoc_opt ename config.enum_prefixes with
|
||||||
| None ->
|
| None ->
|
||||||
say ename
|
mapping_say ename
|
||||||
"the defenum %s has no `enum` line in the package's `bindings`, so \
|
"the defenum %s has no `enum` line in the package's `bindings`, so \
|
||||||
nothing checks its members against the header — add `enum %s \
|
nothing checks its members against the header — add `enum %s \
|
||||||
<C_PREFIX>`, or `enum %s -` to say the header has nothing to \
|
<C_PREFIX>`, or `enum %s -` to say the header has nothing to \
|
||||||
@ -1007,18 +1027,22 @@ let check_constants ~config
|
|||||||
in
|
in
|
||||||
List.iter
|
List.iter
|
||||||
(fun (n, e) ->
|
(fun (n, e) ->
|
||||||
|
(* The prefix is marked as reaching something *before* an explicit
|
||||||
|
[constant] line is consulted, so a rule whose every match is also
|
||||||
|
spelled out by hand is not reported as reaching nothing. A false
|
||||||
|
finding in a check whose whole value is that a finding is real. *)
|
||||||
|
let by_rule =
|
||||||
|
List.find_map
|
||||||
|
(fun (fp, cp) ->
|
||||||
|
match strip_prefix fp n with
|
||||||
|
| Some rest ->
|
||||||
|
Hashtbl.replace used ("const:" ^ fp) ();
|
||||||
|
Some (cp ^ screaming rest)
|
||||||
|
| None -> None)
|
||||||
|
config.const_prefixes
|
||||||
|
in
|
||||||
let cname =
|
let cname =
|
||||||
match List.assoc_opt n explicit with
|
match List.assoc_opt n explicit with Some c -> Some c | None -> by_rule
|
||||||
| Some c -> Some c
|
|
||||||
| None ->
|
|
||||||
List.find_map
|
|
||||||
(fun (fp, cp) ->
|
|
||||||
match strip_prefix fp n with
|
|
||||||
| Some rest ->
|
|
||||||
Hashtbl.replace used ("const:" ^ fp) ();
|
|
||||||
Some (cp ^ screaming rest)
|
|
||||||
| None -> None)
|
|
||||||
config.const_prefixes
|
|
||||||
in
|
in
|
||||||
match cname with
|
match cname with
|
||||||
| None -> ()
|
| None -> ()
|
||||||
@ -1037,14 +1061,14 @@ let check_constants ~config
|
|||||||
List.iter
|
List.iter
|
||||||
(fun (ename, _) ->
|
(fun (ename, _) ->
|
||||||
if not (Hashtbl.mem used ("enum:" ^ ename)) then
|
if not (Hashtbl.mem used ("enum:" ^ ename)) then
|
||||||
say ename
|
mapping_say ename
|
||||||
"`enum %s` in the package's `bindings` names no defenum the \
|
"`enum %s` in the package's `bindings` names no defenum the \
|
||||||
package declares" ename)
|
package declares" ename)
|
||||||
config.enum_prefixes;
|
config.enum_prefixes;
|
||||||
List.iter
|
List.iter
|
||||||
(fun (fp, _) ->
|
(fun (fp, _) ->
|
||||||
if not (Hashtbl.mem used ("const:" ^ fp)) then
|
if not (Hashtbl.mem used ("const:" ^ fp)) then
|
||||||
say fp
|
mapping_say fp
|
||||||
"`const %s` in the package's `bindings` matches no defconst the \
|
"`const %s` in the package's `bindings` matches no defconst the \
|
||||||
package declares" fp)
|
package declares" fp)
|
||||||
config.const_prefixes;
|
config.const_prefixes;
|
||||||
@ -1060,7 +1084,7 @@ let check_constants ~config
|
|||||||
enums
|
enums
|
||||||
in
|
in
|
||||||
if not known then
|
if not known then
|
||||||
say flan
|
mapping_say flan
|
||||||
"`constant %s` in the package's `bindings` names no defconst and no \
|
"`constant %s` in the package's `bindings` names no defconst and no \
|
||||||
enum member the package declares" flan)
|
enum member the package declares" flan)
|
||||||
explicit;
|
explicit;
|
||||||
@ -1391,7 +1415,7 @@ type regen = {
|
|||||||
ghidden : (string * string) list;
|
ghidden : (string * string) list;
|
||||||
gstructs : (string * string) list;
|
gstructs : (string * string) list;
|
||||||
gsigs : sig_diff list;
|
gsigs : sig_diff list;
|
||||||
gconsts : (string * string) list;
|
gconsts : const_diff list;
|
||||||
}
|
}
|
||||||
|
|
||||||
let banner h =
|
let banner h =
|
||||||
|
|||||||
15
lib/load.ml
15
lib/load.ml
@ -876,9 +876,18 @@ let rec import ~seen ~open_ ~loc alias dir =
|
|||||||
ds
|
ds
|
||||||
in
|
in
|
||||||
List.iter
|
List.iter
|
||||||
(fun (n, why) ->
|
(fun (x : Cimport.const_diff) ->
|
||||||
fail (Option.value ~default:loc (cloc n))
|
(* Only the findings that are the *library* contradicting the
|
||||||
"the package disagrees with %s: %s" h why)
|
package stop a build. A [defenum] nobody mapped and a rule
|
||||||
|
that reaches nothing are about the package's own
|
||||||
|
`bindings` file, and they gate `flan generate-c`, which is
|
||||||
|
where that file is edited — telling a lane that added an
|
||||||
|
enum to go and fix a config, in a message shaped like "the
|
||||||
|
layout is wrong", is the wrong thing to stop a build
|
||||||
|
with. *)
|
||||||
|
if not x.Cimport.cmapping then
|
||||||
|
fail (Option.value ~default:loc (cloc x.Cimport.cname))
|
||||||
|
"the package disagrees with %s: %s" h x.Cimport.cwhy)
|
||||||
(Cimport.check_constants ~config ~enums ~consts:pconsts dump);
|
(Cimport.check_constants ~config ~enums ~consts:pconsts dump);
|
||||||
r)
|
r)
|
||||||
(header_specs ~loc dir)
|
(header_specs ~loc dir)
|
||||||
|
|||||||
@ -1724,10 +1724,15 @@ let () =
|
|||||||
prefix rule cannot reach it. The narrow exception, said once. *)
|
prefix rule cannot reach it. The narrow exception, said once. *)
|
||||||
constants = [ ("Shading/half-dark", "SHADE_HALFDARK") ] }
|
constants = [ ("Shading/half-dark", "SHADE_HALFDARK") ] }
|
||||||
in
|
in
|
||||||
let constants ?(config = mapping) src =
|
let raw_constants ?(config = mapping) src =
|
||||||
Cimport.check_constants ~config ~enums:(enums_of (program src))
|
Cimport.check_constants ~config ~enums:(enums_of (program src))
|
||||||
~consts:(pconsts_of (program src)) dump
|
~consts:(pconsts_of (program src)) dump
|
||||||
in
|
in
|
||||||
|
let constants ?(config = mapping) src =
|
||||||
|
List.map
|
||||||
|
(fun (x : Cimport.const_diff) -> (x.Cimport.cname, x.Cimport.cwhy))
|
||||||
|
(raw_constants ~config src)
|
||||||
|
in
|
||||||
check "constants that agree with the header are not reported"
|
check "constants that agree with the header are not reported"
|
||||||
(constants (const_fixture ()) = []);
|
(constants (const_fixture ()) = []);
|
||||||
(* The value is compared, which is the whole point: 340 is KEY_LEFT_SHIFT
|
(* The value is compared, which is the whole point: 340 is KEY_LEFT_SHIFT
|
||||||
@ -1822,6 +1827,19 @@ let () =
|
|||||||
| [ ("Mood/nope", why) ] -> contains why "names no defconst"
|
| [ ("Mood/nope", why) ] -> contains why "names no defconst"
|
||||||
| _ -> false);
|
| _ -> false);
|
||||||
|
|
||||||
|
(* The two kinds of finding are told apart, because they have different
|
||||||
|
dispositions: a value that disagrees with the library stops an ordinary
|
||||||
|
build, and an enum nobody wrote a line for is about the package's own
|
||||||
|
config and gates `generate-c` instead. *)
|
||||||
|
check "a value disagreement is not a mapping finding"
|
||||||
|
(match raw_constants (const_fixture ~fancy:"8" ()) with
|
||||||
|
| [ x ] -> not x.Cimport.cmapping
|
||||||
|
| _ -> false);
|
||||||
|
check "an unmapped defenum is a mapping finding"
|
||||||
|
(match raw_constants (const_fixture ~extra:"(defenum Nobody [a 0])\n" ()) with
|
||||||
|
| [ x ] -> x.Cimport.cmapping
|
||||||
|
| _ -> false);
|
||||||
|
|
||||||
(* The name rule, which is not an inverse of kebab and does not need to be:
|
(* The name rule, which is not an inverse of kebab and does not need to be:
|
||||||
a constant has no declaration to store its C spelling in. *)
|
a constant has no declaration to store its C spelling in. *)
|
||||||
List.iter
|
List.iter
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user