Skip to content

Conversation

@davepacheco
Copy link
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
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.

Copy link
Contributor

@plotnick plotnick left a comment

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.

Copy link
Contributor

@david-crespo david-crespo left a comment

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.

4 participants