Page MenuHomePhabricator

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

Authored by Meinersbur on Aug 13 2020, 2:33 PM.

Details

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.

Diff Detail

Event Timeline

Meinersbur created this revision.Aug 13 2020, 2:33 PM
Herald added a project: Restricted Project. · View Herald Transcript
Meinersbur requested review of this revision.Aug 13 2020, 2:33 PM
DavidTruby accepted this revision.Aug 14 2020, 5:44 AM

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.

This revision is now accepted and ready to land.Aug 14 2020, 5:44 AM
Meinersbur added a comment.EditedAug 14 2020, 1:25 PM

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.

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

Add missed constexpr

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?

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.

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?

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?

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

No.

klausler accepted this revision.Aug 14 2020, 4:06 PM

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

No.

Okay, thanks for checking. Ship it.

  • Make workaround conditional to msvc (requested by @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.

@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 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.

Reverting to the version without ifdefs

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