This is an archive of the discontinued LLVM Phabricator instance.

Work around a Visual C++ bug
ClosedPublic

Authored by probinson on May 21 2019, 8:17 AM.

Details

Summary

Put up for review because I'm not clear whether this should be considered a permanent fix, or if I should put some sort of comment here indicating it's a workaround.

Diff Detail

Event Timeline

probinson created this revision.May 21 2019, 8:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 21 2019, 8:17 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

To provide some missing details:
The original source gets a bogus compile-time error with Visual Studio 2017, prior to version 15.8. I have 15.2, so I need this patch to build Clang.
Our current minimum-version requirement (per CheckCompilerVersion.cmake) is Visual Studio 2015 (some specific update), which assumes all Visual Studio 2017 versions are usable. So by that criterion, we need this patch for all Visual Studio 2017 versions to be usable.
Arguably the anonymous-namespace version is the "more modern" tactic anyway, so as a long-term fix this is actually fine anyway.

My hesitation is... it *is* a compiler bug, and I'm working around it... wondered what y'all think about it.

To provide some missing details:
The original source gets a bogus compile-time error with Visual Studio 2017, prior to version 15.8. I have 15.2, so I need this patch to build Clang.
Our current minimum-version requirement (per CheckCompilerVersion.cmake) is Visual Studio 2015 (some specific update), which assumes all Visual Studio 2017 versions are usable. So by that criterion, we need this patch for all Visual Studio 2017 versions to be usable.
Arguably the anonymous-namespace version is the "more modern" tactic anyway, so as a long-term fix this is actually fine anyway.

Technically this violates the LLVM style guide which says "make anonymous namespaces as small as possible, and only use them for class declarations." (preferring static for functions) - https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

But making code work for the compilers we say we support seems reasonable to me.

What's the compiler bug - can't handle static functions as non-type template parameters in general? Are there other workarounds to consider?

My hesitation is... it *is* a compiler bug, and I'm working around it... wondered what y'all think about it.

Technically this violates the LLVM style guide which says "make anonymous namespaces as small as possible, and only use them for class declarations." (preferring static for functions) - https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

Which argues for flagging it somehow.

But making code work for the compilers we say we support seems reasonable to me.

What's the compiler bug - can't handle static functions as non-type template parameters in general? Are there other workarounds to consider?

Correct, static function was not permitted as a non-type template parameter.
https://developercommunity.visualstudio.com/content/problem/25334/error-code-c2971-when-specifying-a-function-as-the.html
I'm not aware of other workarounds, but I don't claim to be deeply familiar with the darker corners of C++.

Technically this violates the LLVM style guide which says "make anonymous namespaces as small as possible, and only use them for class declarations." (preferring static for functions) - https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

Which argues for flagging it somehow.

Yeah, if we're going this way I'd certainly advocate having a comment of some kind explaining why it's this way so it doesn't regress.

But making code work for the compilers we say we support seems reasonable to me.

What's the compiler bug - can't handle static functions as non-type template parameters in general? Are there other workarounds to consider?

Correct, static function was not permitted as a non-type template parameter.
https://developercommunity.visualstudio.com/content/problem/25334/error-code-c2971-when-specifying-a-function-as-the.html
I'm not aware of other workarounds, but I don't claim to be deeply familiar with the darker corners of C++.

I'm wondering if the code itself could be changed somewhat. Non-type template parameters, especially pointer typed ones (especially especially function pointer typed ones), are pretty rare/unusual. It'd be nice, for instance, to be able to pass a(n ideally stateless) lambda but there's no way to default construct them, or get a constexpr function pointer for them until C++17, unfortunately. (& even then you can't use lambdas directly in a template parameter expression, which is unfortunate... probably because mangling?)

Another possibility to keep these helper functions close to their use would be to put them in local classes inside the functions they're helping - and make the utility function a static member of that class? (it's awkward, but it's no more lines than wrapping each one in an anonymous namespace, etc)

The helper is passed as a template parameter to a class ctor, and that instantiated class calls the helper from operator(), so I suppose maybe enough indirection through lambdas....
but this seems kind of involved for getting my builds to work.

rnk added a comment.May 21 2019, 12:53 PM

I'd go ahead and commit the workaround, even if it goes against the style guide. This problem will go away in two years or so.

rnk added a comment.May 21 2019, 12:56 PM

Yeah, if we're going this way I'd certainly advocate having a comment of some kind explaining why it's this way so it doesn't regress.

In the past, I've found these sorts of comments to be pretty low value. They don't do anything to help the reader understand what the code is trying to do. If we want this thing to not regress, someone who cares can set up a buildbot for it. I think it's more likely that by the time somebody does this again, Paul and other users of Visual C++ will have upgraded to 14.8. Or, they'll send another one of these changes.

In D62202#1510835, @rnk wrote:

Yeah, if we're going this way I'd certainly advocate having a comment of some kind explaining why it's this way so it doesn't regress.

In the past, I've found these sorts of comments to be pretty low value. They don't do anything to help the reader understand what the code is trying to do. If we want this thing to not regress, someone who cares can set up a buildbot for it. I think it's more likely that by the time somebody does this again, Paul and other users of Visual C++ will have upgraded to 14.8. Or, they'll send another one of these changes.

Then a comment might help explain that the workaround can be removed at that point.

// FIXME: Change the following functions from anonymous namespace to static
// after the minimum _MSC_VER >= 1915 (equivalent to Visual Studio version
// of 15.8 or higher).  Works around a bug in earlier versions.

?

Also happy to include the hyperlink to the bug report (given in a previous reply) in the commit log.

dblaikie accepted this revision.May 22 2019, 3:13 PM

SGTM - thanks!

This revision is now accepted and ready to land.May 22 2019, 3:13 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2019, 8:08 AM