[flang][msvc] Split class declaration and constexpr variable definition. NFC.

[Meinersbur]

Summary

Msvc has trouble defining a struct/class and defining a constexpr symbol in the same declarator. It reports the following error:

basic-parsers.h(809): error C2131: expression did not evaluate to a constant
basic-parsers.h(809): note: failure was caused by call of undefined function or one not declared 'constexpr'
basic-parsers.h(809): note: see usage of 'Fortran::parser::OkParser::OkParser'

Fix the msvc compilation by splitting the two definitions into two separate declarators.

This patch is part of the series to make flang compilable with MS Visual Studio.

[DavidTruby]

LGTM!

As an aside would it be worth throwing a bug report at MSVC? I think the existing code is conformant so they might like to know about the bug.

[Meinersbur]

Reported here: https://developercommunity.visualstudio.com/content/problem/1151042/class-with-constexpr-constructor-and-variable-declaration-in-same-declarator.html

[Meinersbur]

The pre-merge check

clang-tidy: warning: variable 'digitString64' defined in a header file; variable definitions in header files can lead to ODR violations [misc-definitions-in-headers]

is correct: I forgot the constexpr for the variable declaration here. Going to update the patch.

[klausler]

You might have to use conditional preprocessing to make this workaround specific to MSVC.

[Meinersbur]

Add missed constexpr

[Meinersbur]

In D85937#2219028, @klausler wrote:

You might have to use conditional preprocessing to make this workaround specific to MSVC.

The workaround is valid C++ and works fine with every compiler. So why add a #if maze?

[DavidTruby]

In D85937#2219052, @Meinersbur wrote:

In D85937#2219028, @klausler wrote:

You might have to use conditional preprocessing to make this workaround specific to MSVC.

The workaround is valid C++ and works fine with every compiler. So why add a #if maze?

I also believe it will generate exactly the same code. There's no functional difference between the new proposal and the old code as far as the standard is concerned. I agree that preprocessor changes aren't necessary here.

[klausler]

In D85937#2219052, @Meinersbur wrote:

In D85937#2219028, @klausler wrote:

You might have to use conditional preprocessing to make this workaround specific to MSVC.

The workaround is valid C++ and works fine with every compiler. So why add a #if maze?

Because you're working around a bug that might get fixed in MSVC, and then the conditional preprocessing could be removed. And if you run into even one MSVC bug later that can't be worked around now, and we have to wait for a later MSVC version, this and other workarounds may no longer be necessary with that later compiler.

I ran into many bugs in GCC 7.2 when implementing the f18 parser. Most of them I worked around when the replacement code was no less readable or maintainable; sometimes, I used conditional code when the replacements were not what I wanted to remain as permanent implementations. It's always a case-by-case call.

In this specific case, the workaround doesn't look too bad. But are there other workarounds? Would adding "static" to the original "constexpr" suffice to avoid the bug?

[klausler]

In D85937#2219151, @DavidTruby wrote:

In D85937#2219052, @Meinersbur wrote:

In D85937#2219028, @klausler wrote:

You might have to use conditional preprocessing to make this workaround specific to MSVC.

The workaround is valid C++ and works fine with every compiler. So why add a #if maze?

I also believe it will generate exactly the same code. There's no functional difference between the new proposal and the old code as far as the standard is concerned. I agree that preprocessor changes aren't necessary here.

Until somebody compiles the changed code with a real compiler and measures no significant difference in performance, we don't know that there isn't one. (I'd be surprised as well if there were a difference in this case, of course, but the parser is quite the stressful case for C++ compilers and some innocuous-looking changes have turned out to matter.) Is anybody benchmarking the effects of Michael's unconditional workarounds in f18's performance, or planning to do so?

[Meinersbur]

In D85937#2219266, @klausler wrote:

But are there other workarounds? Would adding "static" to the original "constexpr" suffice to avoid the bug?

No.

[klausler]

In D85937#2219318, @Meinersbur wrote:

In D85937#2219266, @klausler wrote:

But are there other workarounds? Would adding "static" to the original "constexpr" suffice to avoid the bug?

No.

Okay, thanks for checking. Ship it.

[Meinersbur]

• Make workaround conditional to msvc (requested by @klausler)

[klausler]

Again, I don't think that you have to do the conditional code if you don't want to. Use your own best judgment and be aware of the tradeoffs.

[Meinersbur]

@klausler I updated the patch before I noticed you accepted the version without the requested change that you defended vigorously. Which version do you want?

[klausler]

In D85937#2219412, @Meinersbur wrote:

@klausler I updated the patch before I noticed you accepted the version without the requested change that you defended vigorously. Which version do you want?

That's a mischaracterization of my comments. I suggested alternatives and tried to weigh their tradeoffs.

If you don't care either way, make this and later workarounds conditional. But if you have a preference, use your best judgment and follow it, here and on later MSVC workaround patches.

[Meinersbur]

Reverting to the version without ifdefs

I much prefer avoiding using the preprocessor for a workaround which IMHO does not have any downsides.

flang/lib/Parser/basic-parsers.h

 inline constexpr auto nonemptySeparated(PA p, PB sep) {
   return NonemptySeparated<PA, PB>{p, sep};
 }
 
 // ok is a parser that always succeeds.  It is useful when a parser
 // must discard its result in order to be compatible in type with other
 // parsers in an alternative, e.g. "x >> ok || y >> ok" is type-safe even
 // when x and y have distinct result types.
-constexpr struct OkParser {
+struct OkParser {
   using resultType = Success;
   constexpr OkParser() {}
   static constexpr std::optional<Success> Parse(ParseState &) {
     return Success{};
   }
-} ok;
+};
+constexpr OkParser ok;
 
 // A variant of recovery() above for convenience.
 template <typename PA, typename PB>
 inline constexpr auto localRecovery(MessageFixedText msg, PA pa, PB pb) {
   return recovery(withMessage(msg, pa), pb >> pure<typename PA::resultType>());
 }
 
 // nextCh is a parser that succeeds if the parsing state is not

flang/lib/Parser/expr-parsers.cpp

if (result) {
     }
   }
   return result;
 }
 
 // R1005 add-operand -> [add-operand mult-op] mult-operand
 // R1008 mult-op -> * | /
 // The left recursion in the grammar is implemented iteratively.
-constexpr struct AddOperand {
+struct AddOperand {
   using resultType = Expr;
   constexpr AddOperand() {}
   static inline std::optional<Expr> Parse(ParseState &state) {
     std::optional<Expr> result{multOperand.Parse(state)};
     if (result) {
       auto source{result->source};
       std::function<Expr(Expr &&)> multiply{[&result](Expr &&right) {
         return Expr{
             Expr::Multiply(std::move(result).value(), std::move(right))};
       }};
       std::function<Expr(Expr &&)> divide{[&result](Expr &&right) {
         return Expr{Expr::Divide(std::move(result).value(), std::move(right))};
       }};
       auto more{attempt(sourced("*" >> applyLambda(multiply, multOperand) ||
           "/" >> applyLambda(divide, multOperand)))};
       while (std::optional<Expr> next{more.Parse(state)}) {
         result = std::move(next);
         result->source.ExtendToCover(source);
       }
     }
     return result;
   }
-} addOperand;
+};
+constexpr AddOperand addOperand;
 
 // R1006 level-2-expr -> [[level-2-expr] add-op] add-operand
 // R1009 add-op -> + | -
 // These are left-recursive productions, implemented iteratively.
 // Note that standard Fortran admits a unary + or - to appear only here,
 // by means of a missing first operand; e.g., 2*-3 is valid in C but not
 // standard Fortran.  We accept unary + and - to appear before any primary
 // as an extension.
-constexpr struct Level2Expr {
+struct Level2Expr {
   using resultType = Expr;
   constexpr Level2Expr() {}
   static inline std::optional<Expr> Parse(ParseState &state) {
     static constexpr auto unary{
         sourced(
             construct<Expr>(construct<Expr::UnaryPlus>("+" >> addOperand)) ||
             construct<Expr>(construct<Expr::Negate>("-" >> addOperand))) ||
         addOperand};
if (result) {
           "-" >> applyLambda(subtract, addOperand)))};
       while (std::optional<Expr> next{more.Parse(state)}) {
         result = std::move(next);
         result->source.ExtendToCover(source);
       }
     }
     return result;
   }
-} level2Expr;
+};
+constexpr Level2Expr level2Expr;
 
 // R1010 level-3-expr -> [level-3-expr concat-op] level-2-expr
 // R1011 concat-op -> //
 // Concatenation (//) is left-associative for parsing performance, although
 // one would never notice if it were right-associated.
-constexpr struct Level3Expr {
+struct Level3Expr {
   using resultType = Expr;
   constexpr Level3Expr() {}
   static inline std::optional<Expr> Parse(ParseState &state) {
     std::optional<Expr> result{level2Expr.Parse(state)};
     if (result) {
       auto source{result->source};
       std::function<Expr(Expr &&)> concat{[&result](Expr &&right) {
         return Expr{Expr::Concat(std::move(result).value(), std::move(right))};
       }};
       auto more{attempt(sourced("//" >> applyLambda(concat, level2Expr)))};
       while (std::optional<Expr> next{more.Parse(state)}) {
         result = std::move(next);
         result->source.ExtendToCover(source);
       }
     }
     return result;
   }
-} level3Expr;
+};
+constexpr Level3Expr level3Expr;
 
 // R1012 level-4-expr -> [level-3-expr rel-op] level-3-expr
 // R1013 rel-op ->
 //         .EQ. | .NE. | .LT. | .LE. | .GT. | .GE. |
 //          == | /= | < | <= | > | >=  @ | <>
 // N.B. relations are not recursive (i.e., LOGICAL is not ordered)
-constexpr struct Level4Expr {
+struct Level4Expr {
   using resultType = Expr;
   constexpr Level4Expr() {}
   static inline std::optional<Expr> Parse(ParseState &state) {
     std::optional<Expr> result{level3Expr.Parse(state)};
     if (result) {
       auto source{result->source};
       std::function<Expr(Expr &&)> lt{[&result](Expr &&right) {
         return Expr{Expr::LT(std::move(result).value(), std::move(right))};
if (result) {
               (".GT."_tok || ">"_tok) >> applyLambda(gt, level3Expr)))};
       if (std::optional<Expr> next{more.Parse(state)}) {
         next->source.ExtendToCover(source);
         return next;
       }
     }
     return result;
   }
-} level4Expr;
+};
+constexpr Level4Expr level4Expr;
 
 // R1014 and-operand -> [not-op] level-4-expr
 // R1018 not-op -> .NOT.
 // N.B. Fortran's .NOT. binds less tightly than its comparison operators do.
 // PGI/Intel extension: accept multiple .NOT. operators
-constexpr struct AndOperand {
+struct AndOperand {
   using resultType = Expr;
   constexpr AndOperand() {}
   static inline std::optional<Expr> Parse(ParseState &);
-} andOperand;
+};
+constexpr AndOperand andOperand;
 
 // Match a logical operator or, optionally, its abbreviation.
 inline constexpr auto logicalOp(const char *op, const char *abbrev) {
   return TokenStringMatch{op} ||
       extension<LanguageFeature::LogicalAbbreviations>(
           TokenStringMatch{abbrev});
 }
 
 inline std::optional<Expr> AndOperand::Parse(ParseState &state) {
   static constexpr auto notOp{attempt(logicalOp(".NOT.", ".N.") >> andOperand)};
   if (std::optional<Expr> negation{notOp.Parse(state)}) {
     return Expr{Expr::NOT{std::move(*negation)}};
   } else {
     return level4Expr.Parse(state);
   }
 }
 
 // R1015 or-operand -> [or-operand and-op] and-operand
 // R1019 and-op -> .AND.
 // .AND. is left-associative
-constexpr struct OrOperand {
+struct OrOperand {
   using resultType = Expr;
   constexpr OrOperand() {}
   static inline std::optional<Expr> Parse(ParseState &state) {
     static constexpr auto operand{sourced(andOperand)};
     std::optional<Expr> result{operand.Parse(state)};
     if (result) {
       auto source{result->source};
       std::function<Expr(Expr &&)> logicalAnd{[&result](Expr &&right) {
         return Expr{Expr::AND(std::move(result).value(), std::move(right))};
       }};
       auto more{attempt(sourced(
           logicalOp(".AND.", ".A.") >> applyLambda(logicalAnd, andOperand)))};
       while (std::optional<Expr> next{more.Parse(state)}) {
         result = std::move(next);
         result->source.ExtendToCover(source);
       }
     }
     return result;
   }
-} orOperand;
+};
+constexpr OrOperand orOperand;
 
 // R1016 equiv-operand -> [equiv-operand or-op] or-operand
 // R1020 or-op -> .OR.
 // .OR. is left-associative
-constexpr struct EquivOperand {
+struct EquivOperand {
   using resultType = Expr;
   constexpr EquivOperand() {}
   static inline std::optional<Expr> Parse(ParseState &state) {
     std::optional<Expr> result{orOperand.Parse(state)};
     if (result) {
       auto source{result->source};
       std::function<Expr(Expr &&)> logicalOr{[&result](Expr &&right) {
         return Expr{Expr::OR(std::move(result).value(), std::move(right))};
       }};
       auto more{attempt(sourced(
           logicalOp(".OR.", ".O.") >> applyLambda(logicalOr, orOperand)))};
       while (std::optional<Expr> next{more.Parse(state)}) {
         result = std::move(next);
         result->source.ExtendToCover(source);
       }
     }
     return result;
   }
-} equivOperand;
+};
+constexpr EquivOperand equivOperand;
 
 // R1017 level-5-expr -> [level-5-expr equiv-op] equiv-operand
 // R1021 equiv-op -> .EQV. | .NEQV.
 // Logical equivalence is left-associative.
 // Extension: .XOR. as synonym for .NEQV.
-constexpr struct Level5Expr {
+struct Level5Expr {
   using resultType = Expr;
   constexpr Level5Expr() {}
   static inline std::optional<Expr> Parse(ParseState &state) {
     std::optional<Expr> result{equivOperand.Parse(state)};
     if (result) {
       auto source{result->source};
       std::function<Expr(Expr &&)> eqv{[&result](Expr &&right) {
         return Expr{Expr::EQV(std::move(result).value(), std::move(right))};
       }};
       std::function<Expr(Expr &&)> neqv{[&result](Expr &&right) {
         return Expr{Expr::NEQV(std::move(result).value(), std::move(right))};
       }};
       auto more{attempt(sourced(".EQV." >> applyLambda(eqv, equivOperand) ||
           (".NEQV."_tok ||
               extension<LanguageFeature::XOROperator>(
                   logicalOp(".XOR.", ".X."))) >>
               applyLambda(neqv, equivOperand)))};
       while (std::optional<Expr> next{more.Parse(state)}) {
         result = std::move(next);
         result->source.ExtendToCover(source);
       }
     }
     return result;
   }
-} level5Expr;
+};
+constexpr Level5Expr level5Expr;
 
 // R1022 expr -> [expr defined-binary-op] level-5-expr
 // Defined binary operators associate leftwards.
 template <> std::optional<Expr> Parser<Expr>::Parse(ParseState &state) {
   std::optional<Expr> result{level5Expr.Parse(state)};
   if (result) {
     auto source{result->source};
     std::function<Expr(DefinedOpName &&, Expr &&)> defBinOp{

flang/lib/Parser/token-parsers.h

 constexpr AnyOfChars operator""_ch(const char str[], std::size_t n) {
   return AnyOfChars{SetOfChars(str, n)};
 }
 
 constexpr auto letter{"abcdefghijklmnopqrstuvwxyz"_ch};
 constexpr auto digit{"0123456789"_ch};
 
 // Skips over optional spaces.  Always succeeds.
-constexpr struct Space {
+struct Space {
   using resultType = Success;
   constexpr Space() {}
   static std::optional<Success> Parse(ParseState &state) {
     while (std::optional<const char *> p{state.PeekAtNextChar()}) {
       if (**p != ' ') {
         break;
       }
       state.UncheckedAdvance();
     }
     return {Success{}};
   }
-} space;
+};
+constexpr Space space;
 
 // Skips a space that in free form requires a warning if it precedes a
 // character that could begin an identifier or keyword.  Always succeeds.
 inline void MissingSpace(ParseState &state) {
   if (!state.inFixedForm()) {
     state.Nonstandard(
         LanguageFeature::OptionalFreeFormSpace, "missing space"_en_US);
   }
 }
 
-constexpr struct SpaceCheck {
+struct SpaceCheck {
   using resultType = Success;
   constexpr SpaceCheck() {}
   static std::optional<Success> Parse(ParseState &state) {
     if (std::optional<const char *> p{state.PeekAtNextChar()}) {
       char ch{**p};
       if (ch == ' ') {
         state.UncheckedAdvance();
         return space.Parse(state);
       }
       if (IsLegalInIdentifier(ch)) {
         MissingSpace(state);
       }
     }
     return {Success{}};
   }
-} spaceCheck;
+};
+constexpr SpaceCheck spaceCheck;
 
 // Matches a token string.  Spaces in the token string denote where
 // spaces may appear in the source; they can be made mandatory for
 // some free form keyword sequences.  Missing mandatory spaces in free
 // form elicit a warning; they are not necessary for recognition.
 // Spaces before and after the token are also skipped.
 //
 // Token strings appear in the grammar as C++ user-defined literals
if (content.empty()) {
       return std::nullopt;
     }
     return {std::string{base} + '"' + content + '"'};
   }
 };
 
 // R711 digit-string -> digit [digit]...
 // N.B. not a token -- no space is skipped
-constexpr struct DigitString {
+struct DigitString {
   using resultType = CharBlock;
   static std::optional<resultType> Parse(ParseState &state) {
     if (std::optional<const char *> ch1{state.PeekAtNextChar()}) {
       if (IsDecimalDigit(**ch1)) {
         state.UncheckedAdvance();
         while (std::optional<const char *> p{state.PeekAtNextChar()}) {
           if (!IsDecimalDigit(**p)) {
             break;
           }
           state.UncheckedAdvance();
         }
         return CharBlock{*ch1, state.GetLocation()};
       }
     }
     return std::nullopt;
   }
-} digitString;
+};
+constexpr DigitString digitString;
 
 struct SignedIntLiteralConstantWithoutKind {
   using resultType = CharBlock;
   static std::optional<resultType> Parse(ParseState &state) {
     resultType result{state.GetLocation()};
     static constexpr auto sign{maybe("+-"_ch / space)};
     if (sign.Parse(state)) {
       if (auto digits{digitString.Parse(state)}) {
         result.ExtendToCover(*digits);
         return result;
       }
     }
     return std::nullopt;
   }
 };
 
-constexpr struct DigitString64 {
+struct DigitString64 {
   using resultType = std::uint64_t;
   static std::optional<std::uint64_t> Parse(ParseState &state) {
     std::optional<const char *> firstDigit{digit.Parse(state)};
     if (!firstDigit) {
       return std::nullopt;
     }
     std::uint64_t value = **firstDigit - '0';
     bool overflow{false};
while (auto nextDigit{getDigit.Parse(state)}) {
       }
       value += digitValue;
     }
     if (overflow) {
       state.Say(*firstDigit, "overflow in decimal literal"_err_en_US);
     }
     return {value};
   }
-} digitString64;
+};
+constexpr DigitString64 digitString64;
 
 // R707 signed-int-literal-constant -> [sign] int-literal-constant
 // N.B. Spaces are consumed before and after the sign, since the sign
 // and the int-literal-constant are distinct tokens.  Does not
 // handle a trailing kind parameter.
 static std::optional<std::int64_t> SignedInteger(
     const std::optional<std::uint64_t> &x, Location at, bool negate,
     ParseState &state) {
for (auto j{*charCount}; j-- > 0;) {
           return std::nullopt;
         }
       }
     }
     return content;
   }
 };
 
-constexpr struct ConsumedAllInputParser {
+struct ConsumedAllInputParser {
   using resultType = Success;
   constexpr ConsumedAllInputParser() {}
   static inline std::optional<Success> Parse(ParseState &state) {
     if (state.IsAtEnd()) {
       return {Success{}};
     }
     return std::nullopt;
   }
-} consumedAllInput;
+};
+constexpr ConsumedAllInputParser consumedAllInput;
 
 template <char goal> struct SkipPast {
   using resultType = Success;
   constexpr SkipPast() {}
   constexpr SkipPast(const SkipPast &) {}
   static std::optional<Success> Parse(ParseState &state) {
     while (std::optional<const char *> p{state.GetNextChar()}) {
       if (**p == goal) {
inline constexpr auto optionalListBeforeColons(const PA &p) {
   return "," >> nonemptyList(p) / "::" ||
       ("::"_tok || !","_tok) >> pure<resultType>();
 }
 
 // Skip over empty lines, leading spaces, and some compiler directives (viz.,
 // the ones that specify the source form) that might appear before the
 // next statement.  Skip over empty statements (bare semicolons) when
 // not in strict standard conformance mode.  Always succeeds.
-constexpr struct SkipStuffBeforeStatement {
+struct SkipStuffBeforeStatement {
   using resultType = Success;
   static std::optional<Success> Parse(ParseState &state) {
     if (UserState * ustate{state.userState()}) {
       if (ParsingLog * log{ustate->log()}) {
         // Save memory: vacate the parsing log before each statement unless
         // we're logging the whole parse for debugging.
         if (!ustate->instrumentedParse()) {
           log->clear();
while (std::optional<const char *> at{state.PeekAtNextChar()}) {
               LanguageFeature::EmptyStatement, "empty statement"_en_US)) {
         state.UncheckedAdvance();
       } else {
         break;
       }
     }
     return {Success{}};
   }
-} skipStuffBeforeStatement;
+};
+constexpr SkipStuffBeforeStatement skipStuffBeforeStatement;
 
 // R602 underscore -> _
 constexpr auto underscore{"_"_ch};
 
 // Characters besides letters and digits that may appear in names.
 // N.B. Don't accept an underscore if it is immediately followed by a
 // quotation mark, so that kindParam_"character literal" is parsed properly.
 // PGI and ifort accept '$' in identifiers, even as the initial character.