rust: macros: allow overriding the name of module param constants #877
Conversation
There was a problem hiding this comment.
Looks like a good idea. How about changing one of the examples to use it so that it's running in the CI?
Also, for the syntax what do you think about something like
MODPARAM_FOO_BAR : bool {
command_line_name: "foobar",
default: true,
permissions: 0o644,
description: "distinct const name",
},
where command_line_name is optional? I'm just thinking it makes it clearer what the extra name is for.
|
I wonder if the current API for modparam should be rethought instead. Currently we generate a struct for each param and a const for each generated struct and expose a For example, for a module with two params |
|
Looks like a good idea. How about changing one of the examples
to use it so that it's running in the CI?
Done.
Also, for the syntax what do you think about something like
```
MODPARAM_FOO_BAR : bool {
command_line_name: "foobar",
default: true,
permissions: 0o644,
description: "distinct const name",
},
```
where `command_line_name` is optional? I'm just thinking it
makes it clearer what the extra name is for.
Sounds good, I switched to that approach.
|
phi-gamma
force-pushed
the
module-param-constname
branch
from
108bfcd to
a547a27
Compare
|
@phi-gamma - what do you think about @nbdd0121's suggestion above? I think it wouldn't be that big a change. Instead of creating this struct on each loop, we just create one |
|
@phi-gamma - what do you think about @nbdd0121's suggestion
above? I think it wouldn't be that big a change. Instead of
creating [this struct](https://github.com/Rust-for-Linux/linux/pull/877/files#diff-855d5136d397a3099f455d9cdfdedc1bfbf0508ecbcb6c2357a08502095ded65L441)
on each loop, we just create one `struct` at the end with a
method for each `read_func` generated in the loops.
I agree, I was planning on looking into it over the weekend.
Another improvement I’ve been contemplating is allowing the
key-value arguments to be specified in any order. Right now
they’re a bit deceptive in that they resemble structs but you get
a compile error if you say specify `description` ahead of
`permissions`.
|
|
Yeah, supporting the fields to be in any order would be great too. |
|
I think it's intentional decision by @ojeda |
|
Oh, I didn't realize. I seem to remember we wanted to keep the macros to just the essentials for the initial patch set - was it just that or is there some other reason? |
|
Yeah, we wanted to keep this minimal in the beginning. As for the ordering, the advantage of allowing any order is that it is a bit less annoying to use (harder to write?). The disadvantage is that we will have different orders which is less consistent (harder to read?). |
phi-gamma
force-pushed
the
module-param-constname
branch
from
a547a27 to
bdea1a1
Compare
|
@nbdd0121, @adamrk: Latest revision switched to the single struct version. Maybe a module would be more appropriate than an empty struct? With that struct we’re |
Instead of creating a struct + constant pair for each parameter,
expose the parameters as methods over a single struct `MODPARAM`.
For example the parameter `foobar` becomes available at the Rust
end as `MODPARAM.foobar()`.
The rationale is that currently the `module!` macro generates a
constant with the name of the parameter which can lead to
surprising behavior when attempting to create a let binding of
the same name:
module! {
type: ParamName,
// ...
params: {
foobar : bool {
default: true,
permissions: 0o644,
description: "test",
},
},
}
fn clash() {
let foobar = 42;
}
This causes rustc to error out because `foobar` is treated as a
pattern on the left hand side of the let expression:
10 | / module! {
11 | | type: ParamName,
12 | | name: "param_name",
13 | | author: "A. U. Thor",
... |
21 | | },
22 | | }
| |_- constant defined here
...
25 | let foobar = 42;
| ^^^^^^ -- this expression has type `{integer}`
| |
| expected integer, found struct `__param_name_foobar`
| `foobar` is interpreted as a constant, not a new binding
| help: introduce a new binding instead: `other_foobar`
Essentially, having a parameter `foobar` prevents the creation of
a binding of the same name anywhere else in the module.
Signed-off-by: Philipp Gesang <phg@phi-gamma.net>
Suggested-by: Gary Guo <gary@garyguo.net>
phi-gamma
force-pushed
the
module-param-constname
branch
from
bdea1a1 to
f9a866a
Compare
|
I'd expect that when most Rust devs see a macro that looks like a struct they're going to be surprised that the fields can't be reordered. So I'd support reordering if it can be done cleanly (maybe leave for a separate PR). If that doesn't work out we should probably at least put a warning about it in the doc comment or the compilation error when the macro fails. |
|
Latest version looks good to me. A |
Can’t think of a difference for the author except maybe that you can’t take a reference |
|
From Jonathan Corbet's review of the example module:
Sounds to me like that's a vote in favor of letting the fields be reordered? |
Currently,
module!emits constants to access module parameterswith the same name as the parameter:
Since a
constis visible throughout the file it is defined in,this can cause clashes with
letbindings, e. g. when trying tocreate a binding
let foobar = 42in a module with a parameterof the same name:
Likewise when binding to
foobarin amatchpattern.This PR proposes to extend
module!to allow specifying a distinctname for the constant:
So when loading the module one would still pass
foobar=0but in themodule code the parameter would be accessible as
MODPARAM_FOO_BAR.That would avoid the identifier clashing with pattern matching while
at the same time being more idiomatic Rust.
The
param = PARAM : type { …syntax is just what I came up withwithout giving it much thought. It might as well be a
key: valuesetting though then we’d have to find a name for the key,
constperhaps?
I’m aware of #423 and from a casual glance at the changes (@nbdd0121
please correct me if I’m wrong) it doesn’t appear to address this
issue.