Anastasia added inline comments.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8263
+def err_atomic_init_addressspace : Error<
+  "initialization of atomic variables is restricted to variables in global 
address space">;
 def err_atomic_init_constant : Error<
----------------
bader wrote:
> bader wrote:
> > Anastasia wrote:
> > > echuraev wrote:
> > > > Anastasia wrote:
> > > > > Could we combine this error diag with the one below? I guess they are 
> > > > > semantically very similar apart from one is about initialization and 
> > > > > another is about assignment?
> > > > I'm not sure that it is a good idea to combine these errors. For 
> > > > example, if developer had declared a variable non-constant and not in 
> > > > global address space he would have got the same message for both 
> > > > errors. And it can be difficult to determine what the exact problem is. 
> > > > He can fix one of the problems but he will still get the same error.
> > > Well, I don't actually see that we check for constant anywhere so it's 
> > > also OK if you want to drop this bit. Although I think the original 
> > > intension of this message as I understood was to provide the most 
> > > complete hint.
> > > 
> > > My concern is that these two errors seem to be reporting nearly the same 
> > > issue and ideally we would like to keep diagnostic list as small as 
> > > possible. This also makes the file more concise and messages more 
> > > consistent.
> > I suggest adding a test case with non-constant initialization case to 
> > validate that existing checks cover this case for atomic types already.
> > If so, we can adjust existing diagnostic message to cover both cases: 
> > initialization and assignment expression.
> I don't think it's quite true.
> There are two requirements here that must be met the same time. Atomic 
> variables *declared in the global address space* can be initialized only with 
> "compile time constant'.
> If understand the spec correctly this code is also valid:
> 
>   kernel void foo() {
>     static global atomic_int a = 42; // although it's not clear if we must 
> use ATOMIC_VAR_INIT here.
>     ...
>   }
Precisely, but I think checking for compile time constant should be inherited 
from the general C implementation? I don't think we do anything extra for it.  
Regarding the macro I am not sure we can suitably diagnose it anyways...


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8268
+// Atomics
+def err_atomic_init: Error<
+  "atomic variable can only be assigned to a compile time constant or to 
variables in global adress space">;
----------------
also we should add opencl there:

err_opencl_...


https://reviews.llvm.org/D30643



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to