[PATCH] D83363: [openmp] Use switch in isAllowedClauseForDirective instead of multiple if

[Valentin Clement via Phabricator via llvm-commits]

clementval added a comment.

In D83363#2139526 <https://reviews.llvm.org/D83363#2139526>, @jdenny wrote:

> Other than the last bit of cleanup I commented on, LGTM.


Should be good now.



================
Comment at: llvm/test/TableGen/directive1.td:124
+// IMPL-NEXT:    }
+// IMPL-NEXT:    llvm_unreachable("Invalid Tdl Directive kind");
 // IMPL-NEXT:  }
----------------
jdenny wrote:
> 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.
Of course! Shouldn't try to do two things at the same time ... sorry for the mix. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83363/new/

https://reviews.llvm.org/D83363