From 812bda43f150d818c92dc4409fd915f5021af066 Mon Sep 17 00:00:00 2001
From: peter klausler <pklausler@nvidia.com>
Date: Fri, 7 Jun 2019 16:00:46 -0700
Subject: [PATCH] [flang] Dodge valgrind complaint by cleaning up the grammar a
 bit

Original-commit: flang-compiler/f18@3f9f9af1e8794df628d537c494c8f260bb92c0ca
Reviewed-on: https://github.com/flang-compiler/f18/pull/490
Tree-same-pre-rewrite: false
---
 flang/lib/parser/basic-parsers.h | 16 +++++++++++-----
 flang/lib/parser/grammar.h       |  9 ++++-----
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/flang/lib/parser/basic-parsers.h b/flang/lib/parser/basic-parsers.h
index c12f95b..bff3d51 100644
--- a/flang/lib/parser/basic-parsers.h
+++ b/flang/lib/parser/basic-parsers.h
@@ -49,7 +49,7 @@ namespace Fortran::parser {
 
 // fail<A>("..."_err_en_US) returns a parser that never succeeds.  It reports an
 // error message at the current position.  The result type is unused,
-// but might have to be specified at the point of call for satisfy
+// but might have to be specified at the point of call to satisfy
 // the type checker.  The state remains valid.
 template<typename A> class FailParser {
 public:
@@ -69,7 +69,7 @@ template<typename A = Success> inline constexpr auto fail(MessageFixedText t) {
   return FailParser<A>{t};
 }
 
-// pure(x) returns a parsers that always succeeds, does not advance the
+// pure(x) returns a parser that always succeeds, does not advance the
 // parse, and returns a captured value whose type must be copy-constructible.
 template<typename A> class PureParser {
 public:
@@ -306,7 +306,8 @@ public:
 
 private:
   template<int J>
-  void ParseRest(std::optional<resultType> &result, ParseState &state, ParseState &backtrack) const {
+  void ParseRest(std::optional<resultType> &result, ParseState &state,
+      ParseState &backtrack) const {
     ParseState prevState{std::move(state)};
     state = backtrack;
     result = std::get<J>(ps_).Parse(state);
@@ -695,6 +696,10 @@ inline constexpr auto applyMem(
 // instance of T constructed upon the values they returned.
 // With a single argument that is a parser with no usable value,
 // construct<T>(p) invokes T's default nullary constructor (T(){}).
+// (This means that "construct<T>(Foo >> Bar >> ok)" is functionally
+// equivalent to "Foo >> Bar >> construct<T>()", but I'd like to hold open
+// the opportunity to make construct<> capture source provenance all of the
+// time, and the first form will then lead to better error positioning.)
 
 template<typename RESULT, typename... PARSER, std::size_t... J>
 inline RESULT ApplyHelperConstructor(
@@ -793,10 +798,11 @@ template<bool pass> struct FixedParser {
   using resultType = Success;
   constexpr FixedParser() {}
   static constexpr std::optional<Success> Parse(ParseState &) {
-    if (pass) {
+    if constexpr (pass) {
       return Success{};
+    } else {
+      return std::nullopt;
     }
-    return std::nullopt;
   }
 };
 
diff --git a/flang/lib/parser/grammar.h b/flang/lib/parser/grammar.h
index 0361bfd..7a35e92 100644
--- a/flang/lib/parser/grammar.h
+++ b/flang/lib/parser/grammar.h
@@ -444,10 +444,9 @@ constexpr auto executableConstruct{
 constexpr auto obsoleteExecutionPartConstruct{recovery(ignoredStatementPrefix >>
         fail<ExecutionPartConstruct>(
             "obsolete legacy extension is not supported"_err_en_US),
-    construct<ExecutionPartConstruct>(
+    construct<ExecutionPartConstruct>(construct<ErrorRecovery>(ok /
         statement("REDIMENSION" >> name >>
-            parenthesized(nonemptyList(Parser<AllocateShapeSpec>{})) >> ok) >>
-        construct<ErrorRecovery>()))};
+            parenthesized(nonemptyList(Parser<AllocateShapeSpec>{}))))))};
 
 TYPE_PARSER(recovery(
     withMessage("expected execution part construct"_err_en_US,
@@ -461,8 +460,8 @@ TYPE_PARSER(recovery(
                     statement(indirect(dataStmt))),
                 extension<LanguageFeature::ExecutionPartNamelist>(
                     construct<ExecutionPartConstruct>(
-                        statement(indirect(Parser<NamelistStmt>{}))) ||
-                    obsoleteExecutionPartConstruct)))),
+                        statement(indirect(Parser<NamelistStmt>{})))),
+                obsoleteExecutionPartConstruct))),
     construct<ExecutionPartConstruct>(executionPartErrorRecovery)))
 
 // R509 execution-part -> executable-construct [execution-part-construct]...
-- 
2.7.4