Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

warning cli: tweak single-letter warning deprecation #10312

Merged
merged 8 commits into from
Apr 15, 2021
Merged

warning cli: tweak single-letter warning deprecation #10312

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7921b9a
cli: tweak warning cli deprecation
Octachron Mar 29, 2021
8518de1
review
Octachron Mar 31, 2021
1e4a42c
review: reorganize code and fix a bug
Octachron Apr 14, 2021
1f4c469
review: improved example printing
Octachron Apr 14, 2021
478299a
review: implementation remnant
Octachron Apr 14, 2021
47c38f2
Updated changes for nuanced warning letter deprecation
Octachron Apr 14, 2021
8f5482d
review: tweak comment
Octachron Apr 15, 2021
c21fa45
warning deprecation: add example in the testsuite
Octachron Apr 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Changes
View file
Open in desktop
Expand Up @@ -187,7 +187,7 @@ Working version
- #8877: Call the linker when ocamlopt is invoked with .o and .a files only.
(Greta Yorsh, review by Leo White)

- #10207: deprecate single uppercase or lowercase letter in warning
- #10207, #10312: deprecate single uppercase or lowercase letter in warning
specifications.
The form `-w aBcD` was equivalent to `-w -a+b-c+d`.
It is now deprecated to improve the coexistence with warning mnemonics.
Expand Down
34 changes: 25 additions & 9 deletions utils/warnings.ml
View file
Open in desktop
Expand Up @@ -497,8 +497,14 @@ type token =
| Num of int * int * modifier

let letter_alert tokens =
let deprecated = function Letter (c,None) -> Some c | _ -> None in
let print_char ppf c =
let consecutive_letters (l,current) = function
| Letter (x, None) -> (l, x::current)
| _ ->
match current with
| [] | [ _ ] -> (l,current)
| _ :: _ :: _ -> (List.rev current :: l, [])
in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that this would be easier to read if defined just before the fold_left, with a comment before to explain what we are looking for.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, with a bug fix along the way for "a+bc+d"

let print_warning_char ppf c =
let lowercase = Char.lowercase_ascii c = c in
Format.fprintf ppf "%c%c"
(if lowercase then '-' else '+') c
Expand All @@ -514,15 +520,25 @@ let letter_alert tokens =
else
Format.fprintf ppf "%a%d..%d" print_modifier m a b
| Letter(l,Some m) -> Format.fprintf ppf "%a%c" print_modifier m l
| Letter(l,None) -> print_char ppf l
| Letter(l,None) -> print_warning_char ppf l
in
let consecutive_letters =
let l, on_going = List.fold_left consecutive_letters ([],[]) tokens in
match on_going with
| [] | [_] -> l
| _ :: _ :: _ -> List.rev on_going :: l
in
match List.find_map deprecated tokens with
| None -> None
| Some example ->
match consecutive_letters with
| [] | [] :: _ -> None
| (example :: _ ) :: _ ->
let pos = { Lexing.dummy_pos with pos_fname = "_none_" } in
let nowhere = { loc_start=pos; loc_end=pos; loc_ghost=true } in
let spelling_hint ppf =
if List.length (List.filter_map deprecated tokens) >= 5 then
let max_seq_len =
List.fold_left (fun l x -> max l (List.length x))
0 consecutive_letters
in
if max_seq_len >= 5 then
Format.fprintf ppf
"@ @[Hint: Did you make a spelling mistake \
when using a mnemonic name?@]"
Expand All @@ -531,8 +547,8 @@ let letter_alert tokens =
in
let message =
Format.asprintf
"@[<v>@[Setting a warning with single lowercase \
or uppercase letters, like '%c' or '%c',@ is deprecated.@]@ \
"@[<v>@[Setting a warning with a sequence of lowercase \
or uppercase letters, like '%c%c',@ is deprecated.@]@ \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look right. If I understand correctly, passing the specifier foo would previously result in like 'f' or 'F', which made sense (iirc. we didn't know whether the user used the uppercase or lowercase form?), and now it says like fF, when it should be something like fo or foo.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the first chunk of consecutive unsigned letters seemed simpler and clearer indeed.

@[Use the equivalent signed form:@ %t.@]@ \
@[Hint: Enabling or disabling a warning by its mnemonic name \
requires a + or - prefix.@]\
Expand Down