[PATCH] D15321: [OpenMP 4.0]Parsing and Sema support for 'omp declare target' directive (accelerator support)

[Alexey Bataev via cfe-commits]

ABataev added inline comments.

================
Comment at: lib/Parse/ParseOpenMP.cpp:209
@@ -142,2 +208,3 @@
   case OMPD_taskloop_simd:
+  default:
     Diag(Tok, diag::err_omp_unexpected_directive)
----------------
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

================
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;
   }
----------------
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

================
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;
----------------
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


http://reviews.llvm.org/D15321