[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: clang/include/clang/AST/OpenMPClause.h:2561
+
+  void initFailClause(SourceLocation LParenLoc, OpenMPClauseKind FailParameter,
+                      SourceLocation FailParameterLoc) {
----------------
Unused?


================
Comment at: clang/include/clang/AST/OpenMPClause.h:2590-2597
+  static OMPFailClause *CreateEmpty(const ASTContext &C);
+  static OMPFailClause *Create(const ASTContext &C, SourceLocation StartLoc,
+                               SourceLocation EndLoc);
+  static OMPFailClause *Create(const ASTContext &C,
+                               OpenMPClauseKind FailParameter,
+                               SourceLocation FailParameterLoc,
+                               SourceLocation StartLoc,
----------------
You don't need all these function, this clause is a simple one, it does not require special create function, just new (...) OMPFailClause works in all cases


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12632
 
+void Sema::checkFailClauseParameters(ArrayRef<OMPClause *> Clauses) {
+  for (const OMPClause *C : Clauses) {
----------------
You already has all the info for the fail clause, why do you need to scan through the list of all available clauses to find this fail clause?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12637
+      continue;
+    /* Clauses contains OMPC_fail and the subclause */
+    OpenMPClauseKind FailParameter = FC->getFailParameter();
----------------
Use С++ style comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12708
+      SubClauseLoc = C->getBeginLoc();
+      break;
+    }
----------------
You can perform all required checks for the fail clause here


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:17723
+                                       SourceLocation EndLoc) {
+  return OMPFailClause::Create(Context, StartLoc, EndLoc);
+}
----------------
Just new (Context) OMPFailClause(StartLoc, EndLoc);


================
Comment at: clang/lib/Serialization/ASTReader.cpp:10278
+  case llvm::omp::OMPC_fail:
+    C = OMPFailClause::CreateEmpty(Context);
+    break;
----------------
Just С = new (Context) OMPFailClause();


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123235/new/

https://reviews.llvm.org/D123235