Skip to content

lookup macro could be simpler#1547

Merged
davepacheco merged 1 commit into
mainfrom
lookup-macro-simplify
Aug 5, 2022
Merged

lookup macro could be simpler#1547
davepacheco merged 1 commit into
mainfrom
lookup-macro-simplify

Conversation

@davepacheco

Copy link
Copy Markdown
Collaborator

The lookup_resource! macro used to generate:

struct Foo<'a> {
    key: FooKey<'a>,
}

enum FooKey<'a> {
    Name(...),
    PrimaryKey(...),
    ...
}

There's no need for two types here -- the top-level Foo type could be the enum. The current behavior is a relic of when it used to have more data.

@davepacheco davepacheco requested a review from plotnick August 5, 2022 00:29
@davepacheco

Copy link
Copy Markdown
Collaborator Author

I'm not so sure this is a great idea. Although it does simplify things, it means the enum and its variants are now exposed outside the lookup macro and module.

@plotnick plotnick left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like this change very much; I always found it a bit confusing and slightly un-ergonomic having a separate type for the key. I leave it to you of course, if you think it's not a good idea, but to me the code reads significantly better.

@david-crespo david-crespo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am also in favor

@davepacheco davepacheco merged commit 44b326b into main Aug 5, 2022
@davepacheco davepacheco deleted the lookup-macro-simplify branch August 5, 2022 18:23
@davepacheco davepacheco mentioned this pull request Aug 12, 2022
71 tasks
leftwo pushed a commit that referenced this pull request Jan 27, 2025
Crucible changes are:
Allow read only activation with less than three downstairs (#1608)
Tweaks to automatic flush (#1613)
Update Rust crate twox-hash to v2 (#1547)
Remove `LastFlushAck` (#1603)
Correctly print 'connecting' state (#1612)
Make live-repair part of invariants checks (#1610)
Simplify mend region selection (#1606)
Generic read test for crutest (#1609)
Always remove skipped jobs from dependencies (#1604)
Add libsqlite3-dev install step to Github Actions CI (#1607)
Move Nexus notification to standalone task (#1584)
DTrace cleanup. (#1602)
Reset completed work Downstairs on a `Barrier` operation (#1601)
Upstairs state machine refactoring (3/3) (#1577)

Propolis changes are:
Wire up initial support for AMD perf counters
build: upgrade tokio to 1.40.0 (#836)
build: explicitly install libsqlite3-dev in CI (#834)
add JSON output format to cpuid-gen (#832)
leftwo added a commit that referenced this pull request Jan 27, 2025
Crucible changes are:
Allow read only activation with less than three downstairs (#1608)
Tweaks to automatic flush (#1613)
Update Rust crate twox-hash to v2 (#1547)
Remove `LastFlushAck` (#1603)
Correctly print 'connecting' state (#1612)
Make live-repair part of invariants checks (#1610) Simplify mend region
selection (#1606)
Generic read test for crutest (#1609)
Always remove skipped jobs from dependencies (#1604) Add libsqlite3-dev
install step to Github Actions CI (#1607) Move Nexus notification to
standalone task (#1584) DTrace cleanup. (#1602)
Reset completed work Downstairs on a `Barrier` operation (#1601)
Upstairs state machine refactoring (3/3) (#1577)

Propolis changes are:
Wire up initial support for AMD perf counters
build: upgrade tokio to 1.40.0 (#836)
build: explicitly install libsqlite3-dev in CI (#834) add JSON output
format to cpuid-gen (#832)

---------

Co-authored-by: Alan Hanson <alan@oxide.computer>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants