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
