Add softdeps to module macro #143
Conversation
rust/module.rs
Outdated
| @@ -171,7 +170,14 @@ fn __build_modinfo_string_base( | |||
| } | |||
|
|
|||
| fn __build_modinfo_string_variable(module: &str, field: &str) -> String { | |||
| format!("__{module}_{field}", module = module, field = field) | |||
| static COUNTER: AtomicUsize = AtomicUsize::new(0); | |||
There was a problem hiding this comment.
This seemed a little heavy handed, but currently __MODULE_INFO uses __UNIQUE_ID which adds in __COUNTER__ to the variable name. This was the simplest quick thing I could think of. Without the added count we try to reuse the same name for each softdep.
There was a problem hiding this comment.
Could this function take counter: &mut usize as argument and then pass this around such that each module!() invocation uses the same counter. You can then increment this counter and use it. Proc macros are ideally pure functions of their arguments. The order of invocation of proc macros is not guaranteed in case of rustc and proc macros may be invoked multiple times in case of rust-analyzer. Rustc doesn't care about non-determinism in proc macros at the moment, but rust-analyzer used to crash on non-deterministic proc macros. This has since been fixed, but it is still not a good idea.
There was a problem hiding this comment.
Updated to use a usize here.
|
Just a nit I noticed when trying out the repo. Comments and critiques would be welcomed. |
dlrobertson
force-pushed
the
add-module-softdeps
branch
from
d52dcc3 to
6d34e19
Compare
|
Dan, thanks for this! Having The commit appears to do a reorganization/cleanup at the same time, changing some unrelated lines too (#142 is also doing a couple of those cleanups too). So it would be best to split each logical change into several PRs (or at least several commits).
I'm not sure about this one: having all |
Completely agree. This was just me being lazy and just wanting to get some early thoughts from reviewers.
True. I'm definitely willing to concede this point, but it could increase frustration for new users that have all the entries (name, description, author, etc) set, but do not have them set in the right order. It was confusing for me at least when I first set all of these in a module definition (but didn't have them in the exact order as the docs) and got an error. At a minimum I think there should be a very descriptive message detailing that they must be in the right order. |
|
Agreed! Improving the docs and the error messages is always worth it to avoid making others spend time figuring things out. |
rust/module.rs
Outdated
| "__{module}_{field}{counter}", | ||
| module = module, | ||
| field = field, | ||
| counter = COUNTER.load(Ordering::SeqCst) |
There was a problem hiding this comment.
| counter = COUNTER.load(Ordering::SeqCst) | |
| counter = COUNTER.fetch_add(1, Ordering::SeqCst) |
fetch_add returns the old value.
There was a problem hiding this comment.
👍 good point. that would increase readability
There was a problem hiding this comment.
In the current version this isn't needed.
rust/module.rs
Outdated
| @@ -171,7 +170,14 @@ fn __build_modinfo_string_base( | |||
| } | |||
|
|
|||
| fn __build_modinfo_string_variable(module: &str, field: &str) -> String { | |||
| format!("__{module}_{field}", module = module, field = field) | |||
| static COUNTER: AtomicUsize = AtomicUsize::new(0); | |||
There was a problem hiding this comment.
Could this function take counter: &mut usize as argument and then pass this around such that each module!() invocation uses the same counter. You can then increment this counter and use it. Proc macros are ideally pure functions of their arguments. The order of invocation of proc macros is not guaranteed in case of rustc and proc macros may be invoked multiple times in case of rust-analyzer. Rustc doesn't care about non-determinism in proc macros at the moment, but rust-analyzer used to crash on non-deterministic proc macros. This has since been fixed, but it is still not a good idea.
|
FYI: there's a merge conflict here. |
dlrobertson
force-pushed
the
add-module-softdeps
branch
from
6d34e19 to
cdec777
Compare
|
Updated |
This comment has been minimized.
This comment has been minimized.
Add support for declaring module softdeps in the generated
module info. The module macro may now include something
like the following:
module! {
...
softdeps {
pre: keywrap,
post: sha512,
...
}
Signed-off-by: Dan Robertson <[email protected]>
dlrobertson
force-pushed
the
add-module-softdeps
branch
from
cdec777 to
06759a2
Compare
|
Review of
|
the module! macro
Signed-off-by: Dan Robertson [email protected]