M-Labs/nac3
M-Labs
/
nac3
8
0
Fork 10
Code Issues 100 Pull Requests 20 Releases Wiki Activity

core: Refactor PrimitiveDefinitionIds into an enum + refactor get_builtins() #408

Merged
sb10q merged 1 commits from issue-385-primdef into master 2024-08-17 17:37:21 +08:00
Conversation 5 Commits 1 Files Changed 2 +1416 -1772
lyken commented 2024-06-05 16:43:15 +08:00
Collaborator
Copy Link

Realizes #385 and #386 (comment).

  • In nac3core/src/toplevel/helper.rs, struct PrimitiveDefinitionIds and const PRIMITIVE_DEF_IDS are now replaced with enum PrimitiveDefinition.
  • enum PrimitiveDefinition now also includes primitive functions (See #386 (comment)).
  • Libraries strum and strum_macros are added to implement PrimitiveDefinition::iter with a macro.
Realizes https://git.m-labs.hk/M-Labs/nac3/issues/385 and https://git.m-labs.hk/M-Labs/nac3/pulls/386#issuecomment-9305. - In `nac3core/src/toplevel/helper.rs`, `struct PrimitiveDefinitionIds` and `const PRIMITIVE_DEF_IDS` are now replaced with `enum PrimitiveDefinition`. - `enum PrimitiveDefinition` now also includes primitive functions (See https://git.m-labs.hk/M-Labs/nac3/pulls/386#issuecomment-9305). - Libraries `strum` and `strum_macros` are added to implement `PrimitiveDefinition::iter` with a macro.
sb10q requested review from derppening 2024-06-05 16:47:08 +08:00
lyken force-pushed issue-385-primdef from b6ded9a869 to 3fc8aa8981 2024-06-06 13:02:28 +08:00 Compare
lyken force-pushed issue-385-primdef from 3fc8aa8981 to 84fc379ef4 2024-06-07 10:02:55 +08:00 Compare
derppening reviewed 2024-06-07 13:37:20 +08:00
nac3core/src/toplevel/helper.rs Outdated
@ -24,0 +27,4 @@
OptionUnwrap, // 13
NDArray, // 14
NDArrayCopy, // 15
NDArrayFill, // 16
derppening commented 2024-06-07 13:36:45 +08:00
Collaborator
Copy Link

This doesn't appear to be the entire list of primitive definitions. What about the other Numpy functions?

Also, I would prefer not to comment the ID itself here. Rather, I'd like a test case that asserts that each primitive class/function has the same ID as its index in the Vec returned by get_builtins.

This doesn't appear to be the entire list of primitive definitions. What about the other Numpy functions? Also, I would prefer not to comment the ID itself here. Rather, I'd like a test case that asserts that each primitive class/function has the same ID as its index in the `Vec` returned by `get_builtins`.
lyken commented 2024-06-07 13:45:35 +08:00
Author
Collaborator
Copy Link

This doesn't appear to be the entire list of primitive definitions. What about the other Numpy functions?

I will revise on this.

Also, I would prefer not to comment the ID itself here.

I commented the IDs here since I find it very helpful when printing {:?}s and doing quick lookups to see what is what. Still should I remove them?

Rather, I'd like a test case that asserts that each primitive class/function has the same ID as its index in the Vec returned by get_builtins.

Ok, I will work on this now.

> This doesn't appear to be the entire list of primitive definitions. What about the other Numpy functions? I will revise on this. > Also, I would prefer not to comment the ID itself here. I commented the IDs here since I find it very helpful when printing `{:?}`s and doing quick lookups to see what is what. Still should I remove them? > Rather, I'd like a test case that asserts that each primitive class/function has the same ID as its index in the Vec returned by get_builtins. Ok, I will work on this now.
derppening marked this conversation as resolved
lyken force-pushed issue-385-primdef from 3aaf21fcf9 to 9323877076 2024-06-12 10:55:00 +08:00 Compare
lyken changed title from core: Refactor PrimitiveDefinitionIds into an enum to core: Refactor PrimitiveDefinitionIds into an enum + refactor `get_builtins()` 2024-06-12 11:08:44 +08:00
derppening requested changes 2024-06-12 11:24:10 +08:00
derppening left a comment
Collaborator
Copy Link

The design looks good to me, just several nitpicks here and there.

Other general comments:

  • Please split the formatting changes into a separate commit.
  • Split refactoring PrimitiveDefinitionIds and refactoring get_builtins
  • Run cargo clippy --no-deps -p nac3core -- -Wclippy::pedantic -Aclippy::cast-lossless -Aclippy::cast-possible-truncation -Aclippy::cast-sign-loss -Aclippy::enum_glob_use -Aclippy::implicit-hasher -Aclippy::missing-errors-doc -Aclippy::missing_panics_doc -Aclippy::module-name-repetitions -Aclippy::similar_names -Aclippy::too-many-lines -Aclippy::wildcard_imports and fix all warnings. (I will add a configuration file to make it easier in the future.)
The design looks good to me, just several nitpicks here and there. Other general comments: - Please split the formatting changes into a separate commit. - Split refactoring `PrimitiveDefinitionIds` and refactoring `get_builtins` - Run `cargo clippy --no-deps -p nac3core -- -Wclippy::pedantic -Aclippy::cast-lossless -Aclippy::cast-possible-truncation -Aclippy::cast-sign-loss -Aclippy::enum_glob_use -Aclippy::implicit-hasher -Aclippy::missing-errors-doc -Aclippy::missing_panics_doc -Aclippy::module-name-repetitions -Aclippy::similar_names -Aclippy::too-many-lines -Aclippy::wildcard_imports` and fix all warnings. (I will add a configuration file to make it easier in the future.)
nac3core/src/toplevel/helper.rs Outdated
@ -58,0 +127,4 @@
/// If the [`PrimDef`] is a function, this corresponds to [`TopLevelDef::Function::simple_name`].
///
/// If the [`PrimDef`] is a class, this is equal to [`PrimDef::name`].
pub fn simple_name<'a>(&'a self) -> &'static str {
derppening commented 2024-06-12 11:16:33 +08:00
Collaborator
Copy Link

The lifetime 'a can be elided; Same for the functions below.

The lifetime `'a` can be elided; Same for the functions below.
derppening marked this conversation as resolved
nac3core/src/toplevel/helper.rs Outdated
@ -58,0 +148,4 @@
PrimDefDetails { name, simple_name: name }
}
fn new2(simple_name: &'static str, name: &'static str) -> PrimDefDetails {
derppening commented 2024-06-12 11:10:53 +08:00
Collaborator
Copy Link

Could we just have one constructor with simple_name being an Option<&'static str>?

Could we just have one constructor with `simple_name` being an `Option<&'static str>`?
lyken commented 2024-06-12 11:30:19 +08:00
Author
Collaborator
Copy Link

Sure, I will also change PrimDefDetails to have variants

  • PrimClass { name: &'static str } and.
  • PrimFunction { name: &'static str, simple_name: &'static str }.
Sure, I will also change `PrimDefDetails` to have variants - `PrimClass { name: &'static str }` and. - `PrimFunction { name: &'static str, simple_name: &'static str }`.
derppening marked this conversation as resolved
nac3core/src/toplevel/helper.rs Outdated
@ -76,0 +249,4 @@
///
/// Like `debug_assert!`, this statements of this function are only
/// enabled if `cfg!(debug_assertions)` is true.
pub fn debug_assert_prim_is_allowed<const N: usize>(prim: PrimDef, allowlist: [PrimDef; N]) {
derppening commented 2024-06-12 11:17:10 +08:00
Collaborator
Copy Link

allowlist can just be a &[PrimDef].

`allowlist` can just be a `&[PrimDef]`.
derppening marked this conversation as resolved
lyken commented 2024-06-12 12:19:30 +08:00
Author
Collaborator
Copy Link

All suggested changes are now implemented.

All suggested changes are now implemented.
lyken force-pushed issue-385-primdef from b7bf6ffc5b to b08cd7f8e4 2024-06-12 12:43:58 +08:00 Compare
lyken commented 2024-06-12 12:44:21 +08:00
Author
Collaborator
Copy Link

Force-pushed to fix typo in commit description

Force-pushed to fix typo in commit description
derppening commented 2024-06-12 13:18:52 +08:00
Collaborator
Copy Link

All suggested changes are now implemented.

I think the following changes are not yet implemented:

  • Please split the formatting changes into a separate commit.
  • Split refactoring PrimitiveDefinitionIds and refactoring get_builtins

Do you need some help with that?

> All suggested changes are now implemented. I think the following changes are not yet implemented: - Please split the formatting changes into a separate commit. - Split refactoring PrimitiveDefinitionIds and refactoring get_builtins Do you need some help with that?
lyken force-pushed issue-385-primdef from b08cd7f8e4 to c4420e6ab9 2024-06-12 15:10:22 +08:00 Compare
lyken commented 2024-06-12 15:10:23 +08:00
Author
Collaborator
Copy Link

Revised.

Revised.
derppening commented 2024-06-12 15:44:00 +08:00
Collaborator
Copy Link

LGTM, thanks!

LGTM, thanks!
derppening requested review from sb10q 2024-06-12 15:44:06 +08:00
sb10q merged commit c4420e6ab9 into master 2024-06-12 15:44:33 +08:00
sb10q deleted branch issue-385-primdef 2024-06-12 15:44:33 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
derppening
sb10q
Labels
Clear labels   
future-extension

   
high-priority

   
low-priority

   
UROP0622
No Label
future-extension
high-priority
low-priority
UROP0622
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/nac3#408
No description provided.
Delete Branch "issue-385-primdef"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?