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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
