Skip to content

Try out a different IdKind table approach #5528

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented May 23, 2025

I was thinking about these after #5526, was wondering how others will feel about this kind of approach:

  • Adding a helper to TypeEnum to get the table construction.
  • In what were previously Make functions, return the element instead of returning the table.
  • By returning the element, no more need to pass in a nullptr (now have a concrete instance).

I think this is a mild simplification, but maybe worth it.

Note, would appreciate it if there are thoughts on how to provide a boilerplate Invalid implementation (maybe it'd be fine to just return nullptr and cause a crash that way, but I was hesitant to do that).

@github-actions github-actions bot requested a review from chandlerc May 23, 2025 20:22
@jonmeow
Copy link
Contributor Author

jonmeow commented May 23, 2025

Note, I also tried e.g.:

    static constexpr std::array<AddFnT, SemIR::IdKind::NumValues> Table = {
        [](Worklist& worklist, int32_t arg) {
          worklist.Add(Inst::FromRaw<Types>(arg));
        }...,
        [](Worklist& /*worklist*/, int32_t /*arg*/) {
          CARBON_FATAL("Unexpected invalid argument kind");
        },
        /*none=*/[](Worklist& /*worklist*/, int32_t /*arg*/) {}};

But Clang doesn't like that:

llvm-built/bin/../include/c++/v1/array:191:15: error: '__elems_' declared as array of functions of type 'void (Carbon::SemIR::(anonymous namespace)::Worklist &, int)'
  191 |   _Tp __elems_[_Size];
      |               ^
toolchain/sem_ir/inst_fingerprinter.cpp:320:60: note: in instantiation of template class 'std::array<void (Carbon::SemIR::(anonymous namespace)::Worklist &, int), 41>' requested here
  320 |     static constexpr std::array<AddFnT, IdKind::NumValues> Table = {
      |                                                            ^
1 error generated.

I don't see how to fix this diagnostic. That's why I settled on the MakeTable helper, but maybe I'm missing some syntax detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant