fraggamuffin marked 15 inline comments as done.
fraggamuffin added a comment.
Thanks for the pre-xmas review.
================
Comment at: include/clang/AST/DeclOpenMP.h:98
@@ +97,3 @@
+///
+class OMPDeclareTargetDecl : public Decl, public DeclContext {
+ friend class ASTDeclReader;
----------------
ABataev wrote:
> I'm not sure that we need to add this kind of declaration. Most probably it
> is enough just to have an attribute
I added attributes in a few places according to what was done with
threadprivate but I am not sure it is enough.
================
Comment at: lib/Parse/ParseOpenMP.cpp:209
@@ -142,2 +208,3 @@
case OMPD_taskloop_simd:
+ default:
Diag(Tok, diag::err_omp_unexpected_directive)
----------------
ABataev wrote:
> fraggamuffin wrote:
> > ABataev wrote:
> > > Do not add default:, coding standard recommends to not use it
> > This is needed to generate these error messages in
> > declare_target_messages.cpp
> > error: 'error' diagnostics expected but not seen:
> > File
> > C:\llvm151206\llvm\tools\clang\test\OpenMP\declare_target_messages.cpp Li
> > ne 3: unexpected OpenMP directive '#pragma omp end declare target'
> > File
> > C:\llvm151206\llvm\tools\clang\test\OpenMP\declare_target_messages.cpp Li
> > ne 63: unexpected OpenMP directive '#pragma omp end declare target'
> > File
> > C:\llvm151206\llvm\tools\clang\test\OpenMP\declare_target_messages.cpp Li
> > ne 72: unexpected OpenMP directive '#pragma omp end declare target'
> > 3 errors generated.
> Still 'default:' is a bad solution. You must explicitly take all possible
> cases for the enumeric type
I agree and see your point. Thanks. I fixed it by adding the search for end
declare target.
================
Comment at: lib/Parse/ParseOpenMP.cpp:387-392
@@ -318,2 +386,8 @@
break;
+ default:
+ Diag(Tok, diag::err_omp_unexpected_directive)
+ << getOpenMPDirectiveName(DKind);
+ while (!SkipUntil(tok::annot_pragma_openmp_end))
+ ;
+ break;
}
----------------
ABataev wrote:
> fraggamuffin wrote:
> > ABataev wrote:
> > > Do not add it
> > This is needed to prevent infinite loop when there is an unexpected end of
> > the pragma omp target.
> As I said before 'default:' is a bad solution. Add explicit cases for all
> possible values
I agree and see your point. Thanks. I fixed it .
================
Comment at: lib/Sema/SemaOpenMP.cpp:1139-1140
@@ -1105,3 +1138,4 @@
QualType ExprType = VD->getType().getNonReferenceType();
- ExprResult DE = buildDeclRefExpr(*this, VD, ExprType, Id.getLoc());
+ //ExprResult DE = buildDeclRefExpr(*this, VD, ExprType, Id.getLoc());
+ ExprResult DE = BuildDeclRefExpr(VD, ExprType, VK_LValue, Id.getLoc());
return DE;
----------------
ABataev wrote:
> fraggamuffin wrote:
> > ABataev wrote:
> > > I don't understand why you changed this.
> > The buildDeclRefExpr interface does not enable us to generate these
> > messages.
> > Someone changed it to buildDeclRefExpr(...) which has a different interface
> > In the github repository it also uses
> > ExprResult DE = BuildDeclRefExpr(VD, ExprType, VK_LValue, Id.getLoc());
> >
> > With the buildDeclRefExpr interface, it misses generation of these messages
> > error: 'warning' diagnostics expected but not seen:
> > File
> > C:\llvm151206\llvm\tools\clang\test\OpenMP\declare_target_messages.cpp Li
> > ne 5: declaration is not declared in any declare target region
> > error: 'note' diagnostics expected but not seen:
> > File
> > C:\llvm151206\llvm\tools\clang\test\OpenMP\declare_target_messages.cpp Li
> > ne 21: used here
> > 2 errors generated.
> >
> Github is not a reference version, it has many very bad and not working
> solutions. You don't need take everything from there, some solutions are just
> not compatible with trunk
OK, I looked at the uses of build.. and Build.. and see that build... is only
used within SemaOpenMP.cpp, so I changed it back, and added my check inside
build... for when it is in a target region. Thanks.
http://reviews.llvm.org/D15321
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
