[PATCH] D78577: [OPENMP50]Basic support for uses_allocators clause.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

A few nits and one question/concern below. Once/If that one is addressed this LGTM.



================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:4809
+            << Allocator->getSourceRange();
+    }
     OMPAllocateDeclAttr::AllocatorTypeTy AllocatorKind =
----------------
Note: We should keep the OpenMP context available all the time and ask it for required clauses. That has to be done later though.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:18604
+    Expr *AllocatorExpr = nullptr;
+    // Checkk allocator expression.
+    if (D.Allocator->isTypeDependent()) {
----------------
Nit: Typo


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:18614
+      if (DRE)
+        IsPredefinedAllocator = PredefinedAllocators.count(DRE->getDecl());
+      if (!DRE ||
----------------
Nit: I guess after the condition below these three lines can be placed as `bool IsPredefinedAllocator = PredefinedAllocators.count(DRE->getDecl());`


================
Comment at: clang/test/OpenMP/target_uses_allocators_messages.cpp:54
+}
+
----------------
Do we handle/test the case where the trait array is empty?

```
omp_alloctrait_t empty_traits[0];
```
I guess that doesn't mean much and we should error out?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78577