jdoerfert marked 6 inline comments as done. jdoerfert added a comment. In D74941#1887367 <https://reviews.llvm.org/D74941#1887367>, @fpetrogalli wrote:
> 1. I am not familiar with `OpenMP technical report 8 (TR8)`. Could you please > provide a link in the description? Same for PR42061, PR42798, PR42799. It > would be useful to get an idea of the syntax. Added a link in the commit message. Also to some other commit that might be interesting to see that this is a long time coming. Wrt. the syntax, arguably with this patch clang actually tells you the syntax: https://godbolt.org/z/Cjomow > 2. It is my understanding that this code cover for functionalities that are > still under development in the OpenMP standard. To avoid confusion when > reading the code base, I think it would be worth marking the enumerations, > the error message and (when possible) the methods with an extra descriptor > that marks them as experimental. For example, rename > `OMPD_begin_declare_variant` to `OMPD_experimental_begin_declare_variant`, or > something similar. In the (admittedly unlikely) event the feature doesn't get > in future releases of the standard, it will be easier to clean up the code. I have strong reservations against doing that (here). Let me give you the top two: 1. This will go into 5.1. 2. Things we name "experimental" are traditionally *never* renamed. > 3. This is parsing - why did you have to add also code in the semantic part > of the compiler? Because I wanted to avoid warnings due to non-exhaustive switch statements :) ================ Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1251 "expected '#pragma omp end declare target'">; +def err_expected_end_declare_target_or_variant : Error< + "expected '#pragma omp end declare %select{target|variant}0'">; ---------------- kkwli0 wrote: > Can we merge it with err_expected_end_declare_target? Did that and forgot to delete it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74941/new/ https://reviews.llvm.org/D74941 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
