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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
