r197553 - [OPENMP] Fix for parsing OpenMP directives with extra braces, brackets and parens
Alp Toker
alp at nuanti.com
Wed Dec 18 10:03:49 PST 2013
On 18/12/2013 17:30, Alp Toker wrote:
> 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?
Joey pointed out that Hal did in fact review some of these changes last
month. We're discussing it now to see if the less intrusive approach can
work.
Alp.
>
> 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