[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 2 inline comments as done.
jdoerfert added a comment.

In D69853#1737426 <https://reviews.llvm.org/D69853#1737426>, @ABataev wrote:

> In D69853#1737319 <https://reviews.llvm.org/D69853#1737319>, @jdoerfert wrote:
>
> > In D69853#1737264 <https://reviews.llvm.org/D69853#1737264>, @ABataev wrote:
> >
> > > In D69853#1737218 <https://reviews.llvm.org/D69853#1737218>, @jdoerfert wrote:
> > >
> > > > Are there  blocking issues on this one or can we go ahead and adjust minor details as we go?
> > >
> > >
> > > I still think it would be good to separate patches into the LLVM part and into the clang part and commit them separately? E.g. flang people may try to look for the commits for constant in LLVM, but not for the clang changes. It will be much easier for them to skip the changes in clang.
> >
> >
> > But they might also want to see how to interact with the constants so having the clang parts in there is good. I do not see a clear benefit, e.g., Flang ppl might or might not prefer a separate patch, but I see a clear drawback (testing wise) so I still don't believe splitting the right thing.
>
>
> Testing is not a problem for NFC LLVM part of the patch, I think. Plus, this whole patch is NFC.


That is not a reason not to test it. And again, there are arguments for a combined patch even beyond testing.



================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:4008
+struct Kind2Unsigned {
+  using argument_type = OpenMPDirectiveKind;
+  unsigned operator()(argument_type DK) { return unsigned(DK); }
----------------
ABataev wrote:
> `ArgumentType`
it has to be `argument_type`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853