r197553 - [OPENMP] Fix for parsing OpenMP directives with extra braces, brackets and parens
Alp Toker
alp at nuanti.com
Wed Dec 18 09:30:10 PST 2013
Hi Alexey,
There's a lot I don't understand about your commit so I'm somewhat
uncomfortable here.
I tried reverting your changes to BalancedDelimiterTracker locally and
the tests still pass.
It seems to go against the main purpose of ExpectAndConsume() and
BalancedDelimiterTracker to skip counting braces, and I don't like the
added complexity impacting key C/C++ parse facilities.
Would a simple custom consume-to-end-of-annotation while loop serve your
purpose better?
Also, if you want to diagnose and provide recovery for this error it
shouldn't be specific to 'omp' pragmas. There's a strong expectation
that recovery should work the same and I think that'll fall into place
naturally if you implement this properly instead of the way you did.
So this patch has enough complexity (and issues in its current form,
including the other ones Aaron cited) to roll it out and put it through
pre-commit review. It's entirely possible though that I missed something
-- what do you think?
Alp.
On 18/12/2013 08:46, Alexey Bataev wrote:
> Author: abataev
> Date: Wed Dec 18 02:46:25 2013
> New Revision: 197553
>
> URL: http://llvm.org/viewvc/llvm-project?rev=197553&view=rev
> Log:
> [OPENMP] Fix for parsing OpenMP directives with extra braces, brackets and parens
>
> Modified:
> cfe/trunk/include/clang/Parse/Parser.h
> cfe/trunk/lib/Parse/ParseOpenMP.cpp
> cfe/trunk/lib/Parse/Parser.cpp
> cfe/trunk/lib/Parse/RAIIObjectsForParser.h
> cfe/trunk/test/OpenMP/parallel_messages.cpp
> cfe/trunk/test/OpenMP/threadprivate_messages.cpp
>
> Modified: cfe/trunk/include/clang/Parse/Parser.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Parse/Parser.h?rev=197553&r1=197552&r2=197553&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Parse/Parser.h (original)
> +++ cfe/trunk/include/clang/Parse/Parser.h Wed Dec 18 02:46:25 2013
> @@ -684,9 +684,13 @@ private:
> /// If the input is malformed, this emits the specified diagnostic. Next, if
> /// SkipToTok is specified, it calls SkipUntil(SkipToTok). Finally, true is
> /// returned.
> + /// If NoCount is true, it ignores parens/brackets/braces as regular tokens
> + /// and does not count them. By default it recursively skips properly-nested
> + /// parens/brackets/braces.
> bool ExpectAndConsume(tok::TokenKind ExpectedTok, unsigned Diag,
> const char *DiagMsg = "",
> - tok::TokenKind SkipToTok = tok::unknown);
> + tok::TokenKind SkipToTok = tok::unknown,
> + bool NoCount = false);
>
> /// \brief The parser expects a semicolon and, if present, will consume it.
> ///
> @@ -789,7 +793,9 @@ public:
> StopAtSemi = 1 << 0, ///< Stop skipping at semicolon
> /// \brief Stop skipping at specified token, but don't skip the token itself
> StopBeforeMatch = 1 << 1,
> - StopAtCodeCompletion = 1 << 2 ///< Stop at code completion
> + StopAtCodeCompletion = 1 << 2, ///< Stop at code completion
> + NoBracketsCount = 1 << 3 /// \brief Don't count braces/brackets/parens
> + /// to skip balanced pairs
> };
>
> friend LLVM_CONSTEXPR SkipUntilFlags operator|(SkipUntilFlags L,
>
> Modified: cfe/trunk/lib/Parse/ParseOpenMP.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseOpenMP.cpp?rev=197553&r1=197552&r2=197553&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Parse/ParseOpenMP.cpp (original)
> +++ cfe/trunk/lib/Parse/ParseOpenMP.cpp Wed Dec 18 02:46:25 2013
> @@ -48,7 +48,7 @@ Parser::DeclGroupPtrTy Parser::ParseOpen
> if (Tok.isNot(tok::annot_pragma_openmp_end)) {
> Diag(Tok, diag::warn_omp_extra_tokens_at_eol)
> << getOpenMPDirectiveName(OMPD_threadprivate);
> - SkipUntil(tok::annot_pragma_openmp_end, StopBeforeMatch);
> + SkipUntil(tok::annot_pragma_openmp_end, StopBeforeMatch | NoBracketsCount);
> }
> // Skip the last annot_pragma_openmp_end.
> ConsumeToken();
> @@ -66,7 +66,7 @@ Parser::DeclGroupPtrTy Parser::ParseOpen
> << getOpenMPDirectiveName(DKind);
> break;
> }
> - SkipUntil(tok::annot_pragma_openmp_end);
> + SkipUntil(tok::annot_pragma_openmp_end, NoBracketsCount);
> return DeclGroupPtrTy();
> }
>
> @@ -105,14 +105,14 @@ StmtResult Parser::ParseOpenMPDeclarativ
> if (Tok.isNot(tok::annot_pragma_openmp_end)) {
> Diag(Tok, diag::warn_omp_extra_tokens_at_eol)
> << getOpenMPDirectiveName(OMPD_threadprivate);
> - SkipUntil(tok::annot_pragma_openmp_end, StopBeforeMatch);
> + SkipUntil(tok::annot_pragma_openmp_end, StopBeforeMatch | NoBracketsCount);
> }
> DeclGroupPtrTy Res =
> Actions.ActOnOpenMPThreadprivateDirective(Loc,
> Identifiers);
> Directive = Actions.ActOnDeclStmt(Res, Loc, Tok.getLocation());
> }
> - SkipUntil(tok::annot_pragma_openmp_end);
> + SkipUntil(tok::annot_pragma_openmp_end, NoBracketsCount);
> break;
> case OMPD_parallel: {
> ConsumeToken();
> @@ -171,13 +171,13 @@ StmtResult Parser::ParseOpenMPDeclarativ
> break;
> case OMPD_unknown:
> Diag(Tok, diag::err_omp_unknown_directive);
> - SkipUntil(tok::annot_pragma_openmp_end);
> + SkipUntil(tok::annot_pragma_openmp_end, NoBracketsCount);
> break;
> case OMPD_task:
> case NUM_OPENMP_DIRECTIVES:
> Diag(Tok, diag::err_omp_unexpected_directive)
> << getOpenMPDirectiveName(DKind);
> - SkipUntil(tok::annot_pragma_openmp_end);
> + SkipUntil(tok::annot_pragma_openmp_end, NoBracketsCount);
> break;
> }
> return Directive;
> @@ -194,7 +194,8 @@ bool Parser::ParseOpenMPSimpleVarList(Op
> bool AllowScopeSpecifier) {
> VarList.clear();
> // Parse '('.
> - BalancedDelimiterTracker T(*this, tok::l_paren, tok::annot_pragma_openmp_end);
> + BalancedDelimiterTracker T(*this, tok::l_paren, tok::annot_pragma_openmp_end,
> + true);
> if (T.expectAndConsume(diag::err_expected_lparen_after,
> getOpenMPDirectiveName(Kind)))
> return true;
> @@ -214,17 +215,17 @@ bool Parser::ParseOpenMPSimpleVarList(Op
> ParseOptionalCXXScopeSpecifier(SS, ParsedType(), false)) {
> IsCorrect = false;
> SkipUntil(tok::comma, tok::r_paren, tok::annot_pragma_openmp_end,
> - StopBeforeMatch);
> + StopBeforeMatch | NoBracketsCount);
> } else if (ParseUnqualifiedId(SS, false, false, false, ParsedType(),
> TemplateKWLoc, Name)) {
> IsCorrect = false;
> SkipUntil(tok::comma, tok::r_paren, tok::annot_pragma_openmp_end,
> - StopBeforeMatch);
> + StopBeforeMatch | NoBracketsCount);
> } else if (Tok.isNot(tok::comma) && Tok.isNot(tok::r_paren) &&
> Tok.isNot(tok::annot_pragma_openmp_end)) {
> IsCorrect = false;
> SkipUntil(tok::comma, tok::r_paren, tok::annot_pragma_openmp_end,
> - StopBeforeMatch);
> + StopBeforeMatch | NoBracketsCount);
> Diag(PrevTok.getLocation(), diag::err_expected_ident)
> << SourceRange(PrevTok.getLocation(), PrevTokLocation);
> } else {
> @@ -287,13 +288,13 @@ OMPClause *Parser::ParseOpenMPClause(Ope
> case OMPC_unknown:
> Diag(Tok, diag::warn_omp_extra_tokens_at_eol)
> << getOpenMPDirectiveName(DKind);
> - SkipUntil(tok::annot_pragma_openmp_end, StopBeforeMatch);
> + SkipUntil(tok::annot_pragma_openmp_end, StopBeforeMatch | NoBracketsCount);
> break;
> case OMPC_threadprivate:
> case NUM_OPENMP_CLAUSES:
> Diag(Tok, diag::err_omp_unexpected_clause)
> << getOpenMPClauseName(CKind) << getOpenMPDirectiveName(DKind);
> - SkipUntil(tok::comma, tok::annot_pragma_openmp_end, StopBeforeMatch);
> + SkipUntil(tok::comma, tok::annot_pragma_openmp_end, StopBeforeMatch | NoBracketsCount);
> break;
> }
> return ErrorFound ? 0 : Clause;
> @@ -308,7 +309,8 @@ OMPClause *Parser::ParseOpenMPSimpleClau
> SourceLocation Loc = Tok.getLocation();
> SourceLocation LOpen = ConsumeToken();
> // Parse '('.
> - BalancedDelimiterTracker T(*this, tok::l_paren, tok::annot_pragma_openmp_end);
> + BalancedDelimiterTracker T(*this, tok::l_paren, tok::annot_pragma_openmp_end,
> + true);
> if (T.expectAndConsume(diag::err_expected_lparen_after,
> getOpenMPClauseName(Kind)))
> return 0;
> @@ -342,7 +344,8 @@ OMPClause *Parser::ParseOpenMPVarListCla
> SourceLocation Loc = Tok.getLocation();
> SourceLocation LOpen = ConsumeToken();
> // Parse '('.
> - BalancedDelimiterTracker T(*this, tok::l_paren, tok::annot_pragma_openmp_end);
> + BalancedDelimiterTracker T(*this, tok::l_paren, tok::annot_pragma_openmp_end,
> + true);
> if (T.expectAndConsume(diag::err_expected_lparen_after,
> getOpenMPClauseName(Kind)))
> return 0;
> @@ -357,7 +360,7 @@ OMPClause *Parser::ParseOpenMPVarListCla
> Vars.push_back(VarExpr.take());
> } else {
> SkipUntil(tok::comma, tok::r_paren, tok::annot_pragma_openmp_end,
> - StopBeforeMatch);
> + StopBeforeMatch | NoBracketsCount);
> }
> // Skip ',' if any
> IsComma = Tok.is(tok::comma);
>
> Modified: cfe/trunk/lib/Parse/Parser.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/Parser.cpp?rev=197553&r1=197552&r2=197553&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Parse/Parser.cpp (original)
> +++ cfe/trunk/lib/Parse/Parser.cpp Wed Dec 18 02:46:25 2013
> @@ -159,7 +159,8 @@ static bool IsCommonTypo(tok::TokenKind
> /// SkipToTok is specified, it calls SkipUntil(SkipToTok). Finally, true is
> /// returned.
> bool Parser::ExpectAndConsume(tok::TokenKind ExpectedTok, unsigned DiagID,
> - const char *Msg, tok::TokenKind SkipToTok) {
> + const char *Msg, tok::TokenKind SkipToTok,
> + bool NoCount) {
> if (Tok.is(ExpectedTok) || Tok.is(tok::code_completion)) {
> ConsumeAnyToken();
> return false;
> @@ -189,8 +190,12 @@ bool Parser::ExpectAndConsume(tok::Token
> } else
> Diag(Tok, DiagID) << Msg;
>
> - if (SkipToTok != tok::unknown)
> - SkipUntil(SkipToTok, StopAtSemi);
> + if (SkipToTok != tok::unknown) {
> + SkipUntilFlags Flags = StopAtSemi;
> + if (NoCount)
> + Flags = Flags | NoBracketsCount;
> + SkipUntil(SkipToTok, Flags);
> + }
> return true;
> }
>
> @@ -314,26 +319,32 @@ bool Parser::SkipUntil(ArrayRef<tok::Tok
> case tok::l_paren:
> // Recursively skip properly-nested parens.
> ConsumeParen();
> - if (HasFlagsSet(Flags, StopAtCodeCompletion))
> - SkipUntil(tok::r_paren, StopAtCodeCompletion);
> - else
> - SkipUntil(tok::r_paren);
> + if (!HasFlagsSet(Flags, NoBracketsCount)) {
> + if (HasFlagsSet(Flags, StopAtCodeCompletion))
> + SkipUntil(tok::r_paren, StopAtCodeCompletion);
> + else
> + SkipUntil(tok::r_paren);
> + }
> break;
> case tok::l_square:
> // Recursively skip properly-nested square brackets.
> ConsumeBracket();
> - if (HasFlagsSet(Flags, StopAtCodeCompletion))
> - SkipUntil(tok::r_square, StopAtCodeCompletion);
> - else
> - SkipUntil(tok::r_square);
> + if (!HasFlagsSet(Flags, NoBracketsCount)) {
> + if (HasFlagsSet(Flags, StopAtCodeCompletion))
> + SkipUntil(tok::r_square, StopAtCodeCompletion);
> + else
> + SkipUntil(tok::r_square);
> + }
> break;
> case tok::l_brace:
> // Recursively skip properly-nested braces.
> ConsumeBrace();
> - if (HasFlagsSet(Flags, StopAtCodeCompletion))
> - SkipUntil(tok::r_brace, StopAtCodeCompletion);
> - else
> - SkipUntil(tok::r_brace);
> + if (!HasFlagsSet(Flags, NoBracketsCount)) {
> + if (HasFlagsSet(Flags, StopAtCodeCompletion))
> + SkipUntil(tok::r_brace, StopAtCodeCompletion);
> + else
> + SkipUntil(tok::r_brace);
> + }
> break;
>
> // Okay, we found a ']' or '}' or ')', which we think should be balanced.
> @@ -342,17 +353,17 @@ bool Parser::SkipUntil(ArrayRef<tok::Tok
> // higher level, we will assume that this matches the unbalanced token
> // and return it. Otherwise, this is a spurious RHS token, which we skip.
> case tok::r_paren:
> - if (ParenCount && !isFirstTokenSkipped)
> + if (!HasFlagsSet(Flags, NoBracketsCount) && ParenCount && !isFirstTokenSkipped)
> return false; // Matches something.
> ConsumeParen();
> break;
> case tok::r_square:
> - if (BracketCount && !isFirstTokenSkipped)
> + if (!HasFlagsSet(Flags, NoBracketsCount) && BracketCount && !isFirstTokenSkipped)
> return false; // Matches something.
> ConsumeBracket();
> break;
> case tok::r_brace:
> - if (BraceCount && !isFirstTokenSkipped)
> + if (!HasFlagsSet(Flags, NoBracketsCount) && BraceCount && !isFirstTokenSkipped)
> return false; // Matches something.
> ConsumeBrace();
> break;
> @@ -2031,7 +2042,7 @@ bool BalancedDelimiterTracker::expectAnd
> const char *Msg,
> tok::TokenKind SkipToToc ) {
> LOpen = P.Tok.getLocation();
> - if (P.ExpectAndConsume(Kind, DiagID, Msg, SkipToToc))
> + if (P.ExpectAndConsume(Kind, DiagID, Msg, SkipToToc, NoCount))
> return true;
>
> if (getDepth() < MaxDepth)
> @@ -2056,16 +2067,21 @@ bool BalancedDelimiterTracker::diagnoseM
>
> // If we're not already at some kind of closing bracket, skip to our closing
> // token.
> + Parser::SkipUntilFlags Flags = Parser::StopAtSemi | Parser::StopBeforeMatch;
> + if (NoCount)
> + Flags = Flags | Parser::NoBracketsCount;
> if (P.Tok.isNot(tok::r_paren) && P.Tok.isNot(tok::r_brace) &&
> P.Tok.isNot(tok::r_square) &&
> - P.SkipUntil(Close, FinalToken,
> - Parser::StopAtSemi | Parser::StopBeforeMatch) &&
> + P.SkipUntil(Close, FinalToken, Flags) &&
> P.Tok.is(Close))
> LClose = P.ConsumeAnyToken();
> return true;
> }
>
> void BalancedDelimiterTracker::skipToEnd() {
> - P.SkipUntil(Close, Parser::StopBeforeMatch);
> + Parser::SkipUntilFlags Flags = Parser::StopBeforeMatch;
> + if (NoCount)
> + Flags = Flags | Parser::NoBracketsCount;
> + P.SkipUntil(Close, Flags);
> consumeClose();
> }
>
> Modified: cfe/trunk/lib/Parse/RAIIObjectsForParser.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/RAIIObjectsForParser.h?rev=197553&r1=197552&r2=197553&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Parse/RAIIObjectsForParser.h (original)
> +++ cfe/trunk/lib/Parse/RAIIObjectsForParser.h Wed Dec 18 02:46:25 2013
> @@ -361,6 +361,7 @@ namespace clang {
> tok::TokenKind Kind, Close, FinalToken;
> SourceLocation (Parser::*Consumer)();
> SourceLocation LOpen, LClose;
> + bool NoCount;
>
> unsigned short &getDepth() {
> switch (Kind) {
> @@ -378,9 +379,10 @@ namespace clang {
>
> public:
> BalancedDelimiterTracker(Parser& p, tok::TokenKind k,
> - tok::TokenKind FinalToken = tok::semi)
> + tok::TokenKind FinalToken = tok::semi,
> + bool NoCount = false)
> : GreaterThanIsOperatorScope(p.GreaterThanIsOperator, true),
> - P(p), Kind(k), FinalToken(FinalToken)
> + P(p), Kind(k), FinalToken(FinalToken), NoCount(NoCount)
> {
> switch (Kind) {
> default: llvm_unreachable("Unexpected balanced token");
>
> Modified: cfe/trunk/test/OpenMP/parallel_messages.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/parallel_messages.cpp?rev=197553&r1=197552&r2=197553&view=diff
> ==============================================================================
> --- cfe/trunk/test/OpenMP/parallel_messages.cpp (original)
> +++ cfe/trunk/test/OpenMP/parallel_messages.cpp Wed Dec 18 02:46:25 2013
> @@ -6,6 +6,18 @@ void foo() {
> #pragma omp parallel // expected-error {{unexpected OpenMP directive '#pragma omp parallel'}}
>
> int main(int argc, char **argv) {
> + #pragma omp parallel { // expected-warning {{extra tokens at the end of '#pragma omp parallel' are ignored}}
> + foo();
> + #pragma omp parallel ( // expected-warning {{extra tokens at the end of '#pragma omp parallel' are ignored}}
> + foo();
> + #pragma omp parallel [ // expected-warning {{extra tokens at the end of '#pragma omp parallel' are ignored}}
> + foo();
> + #pragma omp parallel ] // expected-warning {{extra tokens at the end of '#pragma omp parallel' are ignored}}
> + foo();
> + #pragma omp parallel ) // expected-warning {{extra tokens at the end of '#pragma omp parallel' are ignored}}
> + foo();
> + #pragma omp parallel } // expected-warning {{extra tokens at the end of '#pragma omp parallel' are ignored}}
> + foo();
> #pragma omp parallel
> #pragma omp parallel unknown() // expected-warning {{extra tokens at the end of '#pragma omp parallel' are ignored}}
> foo();
>
> Modified: cfe/trunk/test/OpenMP/threadprivate_messages.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/threadprivate_messages.cpp?rev=197553&r1=197552&r2=197553&view=diff
> ==============================================================================
> --- cfe/trunk/test/OpenMP/threadprivate_messages.cpp (original)
> +++ cfe/trunk/test/OpenMP/threadprivate_messages.cpp Wed Dec 18 02:46:25 2013
> @@ -24,6 +24,12 @@ int foo() { // expected-note {{declared
> return (a);
> }
>
> +#pragma omp threadprivate (a) ( // expected-error {{'#pragma omp threadprivate' must precede all references to variable 'a'}} expected-warning {{extra tokens at the end of '#pragma omp threadprivate' are ignored}}
> +#pragma omp threadprivate (a) [ // expected-error {{'#pragma omp threadprivate' must precede all references to variable 'a'}} expected-warning {{extra tokens at the end of '#pragma omp threadprivate' are ignored}}
> +#pragma omp threadprivate (a) { // expected-error {{'#pragma omp threadprivate' must precede all references to variable 'a'}} expected-warning {{extra tokens at the end of '#pragma omp threadprivate' are ignored}}
> +#pragma omp threadprivate (a) ) // expected-error {{'#pragma omp threadprivate' must precede all references to variable 'a'}} expected-warning {{extra tokens at the end of '#pragma omp threadprivate' are ignored}}
> +#pragma omp threadprivate (a) ] // expected-error {{'#pragma omp threadprivate' must precede all references to variable 'a'}} expected-warning {{extra tokens at the end of '#pragma omp threadprivate' are ignored}}
> +#pragma omp threadprivate (a) } // expected-error {{'#pragma omp threadprivate' must precede all references to variable 'a'}} expected-warning {{extra tokens at the end of '#pragma omp threadprivate' are ignored}}
> #pragma omp threadprivate a // expected-error {{expected '(' after 'threadprivate'}}
> #pragma omp threadprivate(d // expected-error {{expected ')'}} expected-note {{to match this '('}} expected-error {{'#pragma omp threadprivate' must precede all references to variable 'd'}}
> #pragma omp threadprivate(d)) // expected-error {{'#pragma omp threadprivate' must precede all references to variable 'd'}} expected-warning {{extra tokens at the end of '#pragma omp threadprivate' are ignored}}
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
--
http://www.nuanti.com
the browser experts
More information about the cfe-commits
mailing list