[PATCH] D83363: [openmp] Use switch in isAllowedClauseForDirective instead of multiple if
Joel E. Denny via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 8 10:18:20 PDT 2020
jdenny accepted this revision.
jdenny added a comment.
This revision is now accepted and ready to land.
Other than the last bit of cleanup I commented on, LGTM.
================
Comment at: llvm/test/TableGen/directive1.td:124
+// IMPL-NEXT: }
+// IMPL-NEXT: llvm_unreachable("Invalid Tdl Directive kind");
// IMPL-NEXT: }
----------------
clementval wrote:
> jdenny wrote:
> > jdenny wrote:
> > > clementval wrote:
> > > > jdenny wrote:
> > > > > The unreachable message doesn't make sense given the `default` in the directive switch. If that switch covers all directives, `default` isn't needed anyway.
> > > > Will remove it.
> > > Is the default useful? Are all directives covered by cases?
> > This is what I'm thinking of:
> >
> > http://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-fully-covered-switches-over-enumerations
> Yeah for directive we get remove it since it's fully covered. Just pushed an update.
But it needs `llvm_unreachable`, whose message makes sense now that `default` is removed.
Also, the last update removed the wrong `default` from the emitter.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83363/new/
https://reviews.llvm.org/D83363
More information about the llvm-commits
mailing list