[PATCH] D112110: [OpenCL] queue_t and ndrange_t can't be defined in program scope.

[Chuang-Yu Cheng via Phabricator via cfe-commits]

cycheng added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:6853
+    if (R->isReserveIDT() || R->isClkEventT() || R->isEventT() ||
+        R.getUnqualifiedType().getAsString() == "ndrange_t" || R->isQueueT()) {
       Se.Diag(NewVD->getLocation(),
----------------
Anastasia wrote:
> Suggest swapping those as string comparisons are more costly.
> 
> However, does it work if there is a typedef of `ndrange_t` to some other name? Although it feels like the same issue might apply to other types diagnosed here...
> does it work if there is a typedef of ndrange_t to some other name?
No, should we recursively check it?
Or do we have better way to handle this?


================
Comment at: clang/test/SemaOpenCL/invalid-types.cl:5
+// Test declare "Other Data Type" variables in program scope.
+global queue_t qt; // expected-error {{the '__global queue_t' type cannot be used to declare a program scope variable}}
+
----------------
Anastasia wrote:
> if you create a type alias through typedef for `queue_t` or `ndrange_t`, will you still get the diagnostic?
> 
Sorry for late reply!
No, I didn't catch that case, I am not sure how to handle this? Could you point out some code for reference?


================
Comment at: clang/test/SemaOpenCL/invalid-types.cl:10
+
+typedef image2d_t test;
+global test qtt; // expected-error {{the '__global ndrange_t' type cannot be used to declare a program scope variable}}
----------------
Anastasia wrote:
> Did you mean to use `ndrange_t` or `queue_t` here? Otherwise, it doesn't seem that your commit is related to the image types...
Ah... sorry it's an accident. I will remove it.


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

https://reviews.llvm.org/D112110