[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:2320-2321
+    : public OMPClause,
+      private llvm::TrailingObjects<OMPFailClause, SourceLocation,
+                                    OpenMPClauseKind> {
+  OMPClause *MemoryOrderClause;
----------------
Why need tail-allocation here for constant number of attributes? They can be represented simply as data members


================
Comment at: clang/include/clang/AST/OpenMPClause.h:2322
+                                    OpenMPClauseKind> {
+  OMPClause *MemoryOrderClause;
+
----------------
`OMPClause *MemoryOrderClause = nullptr;`


================
Comment at: clang/include/clang/AST/OpenMPClause.h:2361
+      : OMPClause(llvm::omp::OMPC_fail, StartLoc, EndLoc) {
+    MemoryOrderClause = 0x0;
+  }
----------------
Remove this


================
Comment at: clang/include/clang/AST/OpenMPClause.h:2367
+      : OMPClause(llvm::omp::OMPC_fail, SourceLocation(), SourceLocation()) {
+    MemoryOrderClause = 0x0;
+  }
----------------
Remove this


================
Comment at: clang/lib/Basic/OpenMPKinds.cpp:446
+      return "seq_cst";
+    default:
+      return "unknown";
----------------
Not recommended to use defaul switch, use all enums explicitly


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:3306
   case OMPC_compare:
+  case OMPC_fail:
   case OMPC_seq_cst:
----------------
Looks like this is the wrong place for this clause, better to parse like  OMPC_default clause


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:3793
+
+  OMPFailClause *FailClause = static_cast<OMPFailClause *>(Clause);
+  SourceLocation LParenLoc;
----------------
auto *FailClause = cast<OMPFailClause>(Clause)


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12614
+
+class OpenMPAtomicFailChecker {
+
----------------
Needs class description


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12615-12616
+class OpenMPAtomicFailChecker {
+
+protected:
+  Sema &SemaRef;
----------------
Why protected?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12621-12622
+public:
+  // Error descriptor type which will be returned to Sema
+  unsigned int ErrorNo;
+
----------------
Should be private member with the default initializer


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12641
+  for (const OMPClause *C : Clauses) {
+    if (const auto *FC = dyn_cast<OMPFailClause>(C)) {
+      NoOfFails++;
----------------
```
const auto *FC = dyn_cast<OMPFailClause>(C)
if (!FC)
  continue;
```
to reduce structural complexity


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12657
+            break;
+          default: break;
+          }
----------------
1. default: cases are not repcommended
2. formatting


================
Comment at: clang/lib/Serialization/ASTReader.cpp:10572-10580
+  case llvm::omp::OMPC_acquire:
+    MemoryOrderClause = new (Context) OMPAcquireClause(SourceLoc, EndLoc);
+    break;
+  case llvm::omp::OMPC_relaxed:
+    MemoryOrderClause = new (Context) OMPRelaxedClause(SourceLoc, EndLoc);
+    break;
+  case llvm::omp::OMPC_seq_cst:
----------------
This is strange you need this


================
Comment at: clang/lib/Serialization/ASTReader.cpp:10581-10582
+    break;
+  default:
+    break;
+  }
----------------
do not use default:


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:6553-6554
+void OMPClauseWriter::VisitOMPFailClause(OMPFailClause *C) {
+  // Record.AddSourceLocation(C->getLParenLoc());
+  // Copied from VisitOMPUpdateClause
+  Record.AddSourceLocation(C->getLParenLoc());
----------------
Remove this


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

https://reviews.llvm.org/D123235