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.

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

Also recognize [@@ocaml.warning] #6714

Closed
vicuna opened this Issue Dec 14, 2014 · 11 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link
Collaborator

vicuna commented Dec 14, 2014

Original bug ID: 6714
Reporter: @whitequark
Assigned to: @whitequark
Status: resolved (set by @gasche on 2014-12-27T08:45:42Z)
Resolution: fixed
Priority: normal
Severity: minor
Category: ~DO NOT USE (was: OCaml general)
Related to: #6677 #7361
Monitored by: @gasche @hcarty

Bug description

Currently, [@ocaml.warning] (on expressions) and [@@@ocaml.warning] (a floating attribute) are recognized. However, it is most often useful to disable warnings per top-level function, not a single expression. Compare actual code from standard library:

with floating attribute, i.e. no well-defined scope:

[@@@ocaml.warning "-3"]
let uppercase s = map Char.uppercase s
let lowercase s = map Char.lowercase s

let capitalize s = apply1 Char.uppercase s
let uncapitalize s = apply1 Char.lowercase s

with expression attribute:

let uppercase s = (map Char.uppercase s)[@ocaml.warning "-3"]
let lowercase s = (map Char.lowercase s)[@ocaml.warning "-3"]

let capitalize s = (apply1 Char.uppercase s)[@ocaml.warning "-3"]
let uncapitalize s = (apply1 Char.lowercase s)[@ocaml.warning "-3"]

with structure item attribute, very nicely scoped:

let uppercase s = map Char.uppercase s
and lowercase s = map Char.lowercase s

and capitalize s = apply1 Char.uppercase s
and uncapitalize s = apply1 Char.lowercase s
[@@ocaml.warning "-3"]

File attachments

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 15, 2014

Comment author: @alainfrisch

In a "let-and" sequence of bindings, each binding gets its own attributes, as in:

let x = 1 [@@foo] and y = 2 [@@bar];;

So in your example, you'd need to repeat the attribute four times as well.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 15, 2014

Comment author: @whitequark

I see. This is still much more convenient (and less invasive) than surrounding the whole body in parentheses or begin..end.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 15, 2014

Comment author: @alainfrisch

I tend to agree, although current workarounds aren't too bad:

include struct
...
end [@ocaml.warning "-3"]

(attribute on the module expression)

or:

include struct [@@@ocaml.warning "-3"]
...
end

(floating attribute scoped to the current structure)

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 19, 2014

Comment author: @whitequark

I wrote a patch and a test (I don't know where to put the test and there are no tests for ocaml.warning already; it can be just run with ocamlc test.ml.)

The patch handles [@@ocaml.warning] on values, modules, module types and includes. The patch doesn't handle [@@ocaml.warning] on types, because most warnings on types are about unused types or constructors and they're ran delayed and thus aren't affected by the attribute anyway, and classes and class types, because I'm not smart enough to modify Typeclass.

Anyway this is an incomplete implementation that's less incomplete than the current incomplete implementation, so it should be good.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 19, 2014

Comment author: @whitequark

Actually, the deprecation warning also appears on types, so [@@ocaml.warning] could be useful there, but Typedecl is as inextricable as Typeclass. I give up on it.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 19, 2014

Comment author: @gasche

You could create a subdirectory tests/ppx-attributes in testsuite/, and imitate what is done in the other directories (the simplest thing to use is Makefile.several, but you might be interested in defining test validation in terms of what happens when the input is passed to the toplevel (Makefile.toplevel) instead of compiled).

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 19, 2014

Comment author: @whitequark

It doesn't matter; it works just as well at toplevel. Would that be simpler?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 19, 2014

Comment author: @gasche

In my experience toplevel tests are a bit less robust, so if both options work for the things you want to test I'd recommend avoiding them.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 21, 2014

Comment author: @whitequark

I've updated the patch to include tests.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 21, 2014

Comment author: @gasche

There is no need to save the warning scope if the attributes do not concern warning (which I believe is the most common situation), but your with_warning_attributes interface allows to optimize that easily if that was ever found to be a concern.

I like the patch.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 27, 2014

Comment author: @gasche

Patch merged in trunk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.