[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

lebedev.ri added a comment. In D59214#1926978 , @sammccall wrote: > In D59214#1919183 , @lebedev.ri > wrote: > > > d5edcb90643104d6911da5c0ff44c4f33fff992f > >

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

sammccall added a comment. In D59214#1919183 , @lebedev.ri wrote: > d5edcb90643104d6911da5c0ff44c4f33fff992f > , > looking forward to seeing better error recovery. Thanks very much fo

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

lebedev.ri added a comment. d5edcb90643104d6911da5c0ff44c4f33fff992f , looking forward to seeing better error recovery. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59214/new/ https://reviews.

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

rjmccall added a comment. In D59214#1917149 , @lebedev.ri wrote: > In D59214#1916596 , @sammccall wrote: > > > So if I understand the history here: > > > > - the `IsOMPStructuredBlock` bit on `Stmt` was added to imp

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

lebedev.ri added a comment. In D59214#1916596 , @sammccall wrote: > So if I understand the history here: > > - the `IsOMPStructuredBlock` bit on `Stmt` was added to implement > `getStructuredBlock()` > - after review, `getStructuredBlock()` doesn't use th

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

rjmccall added a comment. If the bit is unused except for propagation, please remove it. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59214/new/ https://reviews.llvm.org/D59214 ___ cfe-commits mailing list cfe-commit

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

sammccall added a comment. Herald added a reviewer: jdoerfert. So if I understand the history here: - the `IsOMPStructuredBlock` bit on `Stmt` was added to implement `getStructuredBlock()` - after review, `getStructuredBlock()` doesn't use the bit - the bit is used in the textual AST dump, and a

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

rupprecht added inline comments. Comment at: cfe/trunk/unittests/AST/OMPStructuredBlockTest.cpp:58 + std::vector Args = { + "-fopenmp", + }; Looks like this test fails when the default is not libomp, e.g. DCLANG_DEFAULT_OPENMP_RUNTIME=libgomp Using -fopen

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

lebedev.ri added inline comments. Comment at: include/clang/AST/Stmt.h:102 +/// This bit is set only for the Stmt's that are the structured-block +/// of OpeMP [non-standalone] executable directives. +/// I.e. those returned by OMPExecutableDirective::getStructuredBlo

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

ABataev accepted this revision. ABataev added a comment. LG for the OpenMP part Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59214/new/ https://reviews.llvm.org/D59214 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: include/clang/AST/Stmt.h:102 +/// This bit is set only for the Stmt's that are the structured-block +/// of OpeMP [non-standalone] executable di

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

lebedev.ri added inline comments. Comment at: include/clang/AST/Stmt.h:102 +/// This bit is set only for the Stmt's that are the structured-block +/// of OpeMP [non-standalone] executable directives. +/// I.e. those returned by OMPExecutableDirective::getStructuredBlo

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

gribozavr added inline comments. Comment at: unittests/AST/OMPStructuredBlockTest.cpp:64 +++i; +})"; + ASSERT_TRUE( lebedev.ri wrote: > gribozavr wrote: > > Same comment as in the other patch -- I would prefer that the source is > > inlined into the ASSERT_TRUE

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

lebedev.ri added inline comments. Comment at: unittests/AST/OMPStructuredBlockTest.cpp:64 +++i; +})"; + ASSERT_TRUE( gribozavr wrote: > Same comment as in the other patch -- I would prefer that the source is > inlined into the ASSERT_TRUE, with implicit string

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

gribozavr added inline comments. Comment at: unittests/AST/OMPStructuredBlockTest.cpp:64 +++i; +})"; + ASSERT_TRUE( Same comment as in the other patch -- I would prefer that the source is inlined into the ASSERT_TRUE, with implicit string concatenation and "\n"

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

ABataev added a comment. The OpenMP part looks good. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59214/new/ https://reviews.llvm.org/D59214 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

gribozavr added a comment. As far as I'm concerned, I don't have any major revision requests. I haven't reviewed the unit tests (I'm planning to), but for non-test code, I don't have any further comments. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59214/new/

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

lebedev.ri added a comment. Before i do address any further bike^W review notes in other reviews in this path stack, i want to finish with this review. If **this** change doesn't stick, rest is pointless waste of everyone's time. @ABataev please do tell if there is any outstanding issues here. I

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

gribozavr added inline comments. Comment at: include/clang/AST/Stmt.h:102 +/// This bit is set only for the Stmt's that are the structured-block +/// of OpeMP [non-standalone] executable directives. +/// I.e. those returned by OMPExecutableDirective::getStructuredBloc

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

riccibruno added inline comments. Comment at: lib/AST/StmtOpenMP.cpp:40 + if (auto *LD = dyn_cast(this)) +return LD->getBody(); + return getInnermostCapturedStmt()->getCapturedStmt(); lebedev.ri wrote: > riccibruno wrote: > > lebedev.ri wrote: > > > @riccib

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

lebedev.ri added inline comments. Comment at: lib/AST/StmtOpenMP.cpp:40 + if (auto *LD = dyn_cast(this)) +return LD->getBody(); + return getInnermostCapturedStmt()->getCapturedStmt(); riccibruno wrote: > lebedev.ri wrote: > > @riccibruno > > `getBody()` exi

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

riccibruno added inline comments. Comment at: lib/AST/StmtOpenMP.cpp:40 + if (auto *LD = dyn_cast(this)) +return LD->getBody(); + return getInnermostCapturedStmt()->getCapturedStmt(); lebedev.ri wrote: > @riccibruno > `getBody()` exists in `const`-only vari

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

lebedev.ri added inline comments. Comment at: lib/AST/StmtOpenMP.cpp:40 + if (auto *LD = dyn_cast(this)) +return LD->getBody(); + return getInnermostCapturedStmt()->getCapturedStmt(); @riccibruno `getBody()` exists in `const`-only variant `getInnermostCaptu

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

riccibruno added inline comments. Comment at: include/clang/AST/StmtOpenMP.h:274 + /// Prerequisite: Executable Directive must not be Standalone directive. + const Stmt *getStructuredBlock() const; }; Uh, and what about the non-const version of `getStructuredB

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

lebedev.ri added inline comments. Comment at: include/clang/AST/StmtOpenMP.h:274 + /// Prerequisite: Executable Directive must not be Standalone directive. + Stmt *getStructuredBlock() const; }; riccibruno wrote: > This is not const-correct. The const-qualifie

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

riccibruno added inline comments. Comment at: include/clang/AST/Stmt.h:100 unsigned sClass : 8; +unsigned IsOMPStructuredBlock : 1; }; What about a comment explaining the purpose of this bit ? Comment at: include/clang/AST/StmtOpenM

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

lebedev.ri added a comment. In D59214#1429422 , @riccibruno wrote: > In D59214#1429400 , @lebedev.ri > wrote: > > > In D59214#1429384 , @riccibruno > > wrote: > > > > > IIR

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

riccibruno added a comment. In D59214#1429400 , @lebedev.ri wrote: > In D59214#1429384 , @riccibruno > wrote: > > > IIRC, last time I looked only 4 statement/expression classes currently have > > some abbreviation

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

lebedev.ri added a comment. In D59214#1429384 , @riccibruno wrote: > IIRC, last time I looked only 4 statement/expression classes currently have > some abbreviation defined. Yep, that is what i'm seeing in this diff. In D59214#1429384

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

riccibruno added a comment. IIRC, last time I looked only 4 statement/expression classes currently have some abbreviation defined. It would probably be useful to have someone go systematically through the list of classes and fix this. Repository: rC Clang CHANGES SINCE LAST ACTION https:/

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

ABataev added inline comments. Comment at: include/clang/AST/StmtOpenMP.h:335 + llvm::Optional getStructuredBlockImpl() const { +return const_cast(getInnermostCapturedStmt()->getCapturedStmt()); lebedev.ri wrote: > lebedev.ri wrote: > > ABataev wrote: > >

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

lebedev.ri added inline comments. Comment at: include/clang/AST/StmtOpenMP.h:335 + llvm::Optional getStructuredBlockImpl() const { +return const_cast(getInnermostCapturedStmt()->getCapturedStmt()); lebedev.ri wrote: > ABataev wrote: > > lebedev.ri wrote: >

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

gribozavr added inline comments. Comment at: test/AST/ast-dump-openmp-atomic.c:1 +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fopenmp -ast-dump %s | FileCheck -strict-whitespace -implicit-check-not=openmp_structured_block %s + lebedev.ri wrote: > gribozav

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

lebedev.ri added inline comments. Comment at: include/clang/AST/StmtOpenMP.h:335 + llvm::Optional getStructuredBlockImpl() const { +return const_cast(getInnermostCapturedStmt()->getCapturedStmt()); ABataev wrote: > lebedev.ri wrote: > > lebedev.ri wrote: >

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

ABataev added inline comments. Comment at: include/clang/AST/StmtOpenMP.h:335 + llvm::Optional getStructuredBlockImpl() const { +return const_cast(getInnermostCapturedStmt()->getCapturedStmt()); lebedev.ri wrote: > lebedev.ri wrote: > > ABataev wrote: > >

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

lebedev.ri added inline comments. Comment at: include/clang/AST/StmtOpenMP.h:335 + llvm::Optional getStructuredBlockImpl() const { +return const_cast(getInnermostCapturedStmt()->getCapturedStmt()); lebedev.ri wrote: > ABataev wrote: > > lebedev.ri wrote: >

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

lebedev.ri added inline comments. Comment at: test/AST/ast-dump-openmp-atomic.c:1 +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fopenmp -ast-dump %s | FileCheck -strict-whitespace -implicit-check-not=openmp_structured_block %s + gribozavr wrote: > lebedev.

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

lebedev.ri added inline comments. Comment at: include/clang/AST/StmtOpenMP.h:335 + llvm::Optional getStructuredBlockImpl() const { +return const_cast(getInnermostCapturedStmt()->getCapturedStmt()); ABataev wrote: > lebedev.ri wrote: > > ABataev wrote: > >

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

gribozavr added inline comments. Comment at: include/clang/AST/StmtOpenMP.h:269 + /// or if this is an OpenMP stand-alone directive returns `None`. + llvm::Optional getStructuredBlock() const; }; lebedev.ri wrote: > gribozavr wrote: > > Why not `Stmt *` as a r

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

ABataev added inline comments. Comment at: include/clang/AST/StmtOpenMP.h:335 + llvm::Optional getStructuredBlockImpl() const { +return const_cast(getInnermostCapturedStmt()->getCapturedStmt()); lebedev.ri wrote: > ABataev wrote: > > No need to insert it i

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

lebedev.ri added subscribers: steveire, aaron.ballman. lebedev.ri added a comment. Thanks for taking a look, some replies. Comment at: include/clang/AST/StmtOpenMP.h:269 + /// or if this is an OpenMP stand-alone directive returns `None`. + llvm::Optional getStructuredBlock()

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

gribozavr added inline comments. Comment at: include/clang/AST/StmtOpenMP.h:269 + /// or if this is an OpenMP stand-alone directive returns `None`. + llvm::Optional getStructuredBlock() const; }; Why not `Stmt *` as a return value? Repository: rC Clang CH

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

gribozavr added inline comments. Comment at: include/clang/AST/StmtOpenMP.h:266 + + /// Returns the AST statement representing OpenMP structured-block of this + /// OpenMP executable directive, "the AST node" Comment at: include/clang/AST/Stm

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

ABataev added inline comments. Comment at: include/clang/AST/StmtOpenMP.h:335 + llvm::Optional getStructuredBlockImpl() const { +return const_cast(getInnermostCapturedStmt()->getCapturedStmt()); No need to insert it into each class, just add: ``` Stmt * OM