bader 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< ---------------- Anastasia wrote: > 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... > Precisely, but I think checking for compile time constant should be inherited > from the general C implementation? Agree. I suggested checking this above by extending OpenCL tests, but this can be done separately. https://reviews.llvm.org/D30643 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits