Anastasia added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:159
+def err_verify_nonconst_addrspace : Error<
+ "qualifier 'const' is needed for variables in address space '%0'">;
----------------
Ok, do you plan to pass anything other than `__flash` here? If not then you
don't need to pass an argument in this diagnostic.
================
Comment at: clang/test/CodeGen/avr-flash.c:5
+ static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier
'const' is needed for variables in address space '__flash'}}
+ return b[j] + b[i];
+}
----------------
Is there anything you are testing with these array subscript expressions? If
not I suggest removing them and only keep what is needed for testing the new
functionality.
================
Comment at: clang/test/CodeGen/avr-flash.c:1
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+
----------------
benshi001 wrote:
> benshi001 wrote:
> > Anastasia wrote:
> > > If you are only checking the diagnostics you should pass:
> > >
> > > `-emit-llvm-only` -> `-syntax-only`
> > >
> > > and also it should be moved to `clang/test/Sema`.
> > This test can not run with `-syntax-only`. Since the check is performed in
> > the codegen stage.
> >
> > I would like to do the check in the Sema stage, but there is no target
> > specific code in the sema stage. And such checks are better to be put into
> > AVRTargetCodeGenInfo.
> I haved moved all `__flash` related to the CodeGen part, now it has nothing
> to do with `Sema/-syntax-only`.
> I would like to do the check in the Sema stage, but there is no target
> specific code in the sema stage. And such checks are better to be put into
> AVRTargetCodeGenInfo
Sema has target hooks and access to the target specific address spaces so you
could indeed move them to Sema. Normally we do diagnostics as early as possible
but there is no strict rule and there are some tradeoffs. So, you can also keep
it here to allow better code incapsulation and simplify maintenance.
If you need to implement the pointer conversion logic you might move this
completely to `Sema`. Btw just to highlight the following change
https://reviews.llvm.org/D62574 that generalizes some logic that might be
useful for your pointer conversions.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96853/new/
https://reviews.llvm.org/D96853
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
