Work around a Visual C++ bug

[probinson]

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.

[probinson]

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.

[dblaikie]

In D62202#1510394, @probinson wrote:

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.

[probinson]

In D62202#1510414, @dblaikie wrote:

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

[dblaikie]

In D62202#1510449, @probinson wrote:

In D62202#1510414, @dblaikie wrote:

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)

[probinson]

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]

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]

In D62202#1510522, @dblaikie 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.

[dblaikie]

In D62202#1510835, @rnk wrote:

In D62202#1510522, @dblaikie 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.

[probinson]

// 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]

SGTM - thanks!

cfe/trunk/lib/Tooling/Refactoring/RangeSelector.cpp

if (!N)
       return N.takeError();
     if (const auto *Arg = N->get<T>())
       return Func(Result, *Arg);
     return typeError(ID, N->getNodeKind());
   }
 };
 } // namespace
 
+// FIXME: Change the following functions from being in an anonymous namespace
+// to static functions, after the minimum Visual C++ has _MSC_VER >= 1915
+// (equivalent to Visual Studio 2017 v15.8 or higher). Using the anonymous
+// namespace works around a bug in earlier versions.
+namespace {
 // Returns the range of the statements (all source between the braces).
-static CharSourceRange getStatementsRange(const MatchResult &,
-                                          const CompoundStmt &CS) {
+CharSourceRange getStatementsRange(const MatchResult &,
   return CharSourceRange::getCharRange(CS.getLBracLoc().getLocWithOffset(1),
                                        CS.getRBracLoc());
 }
+} // namespace
 
 RangeSelector tooling::statements(StringRef ID) {
   return RelativeSelector<CompoundStmt, getStatementsRange>(ID);
 }
 
+namespace {
 // Returns the range of the source between the call's parentheses.
-static CharSourceRange getCallArgumentsRange(const MatchResult &Result,
-                                             const CallExpr &CE) {
+CharSourceRange getCallArgumentsRange(const MatchResult &Result,
   return CharSourceRange::getCharRange(
       findOpenParen(CE, *Result.SourceManager, Result.Context->getLangOpts())
           .getLocWithOffset(1),
       CE.getRParenLoc());
 }
+} // namespace
 
 RangeSelector tooling::callArgs(StringRef ID) {
   return RelativeSelector<CallExpr, getCallArgumentsRange>(ID);
 }
 
+namespace {
 // Returns the range of the elements of the initializer list. Includes all
 // source between the braces.
-static CharSourceRange getElementsRange(const MatchResult &,
-                                        const InitListExpr &E) {
+CharSourceRange getElementsRange(const MatchResult &,
   return CharSourceRange::getCharRange(E.getLBraceLoc().getLocWithOffset(1),
                                        E.getRBraceLoc());
 }
+} // namespace
 
 RangeSelector tooling::initListElements(StringRef ID) {
   return RelativeSelector<InitListExpr, getElementsRange>(ID);
 }
 
 RangeSelector tooling::expansion(RangeSelector S) {
   return [S](const MatchResult &Result) -> Expected<CharSourceRange> {
     Expected<CharSourceRange> SRange = S(Result);
     if (!SRange)
       return SRange.takeError();
     return Result.SourceManager->getExpansionRange(*SRange);
   };
 }