This is an archive of the discontinued LLVM Phabricator instance.

[flang][msvc] Avoid ambiguous overload from base class. NFC.
AbandonedPublic

Authored by Meinersbur on Aug 22 2020, 4:12 PM.

Details

Summary

The method template <int KIND> Result operator()(const TypeParamInquiry<KIND> &) is defined in evaluate::GetShapeHelper as well as in its ancestor evaluate::Traverse. Normally, only the overloads of class that first defines method name would be considered, but the declaration using Base::operator() imports the base class methods with the same signature. Msvc interprets this as an ambiguity.

This pattern occurs in the following classes:

  • evaluate::GetShapeHelper
  • evaluate::IsConstantExprHelper
  • evaluate::IsInitialDataTargetHelper
  • evaluate::CheckSpecificationExprHelper

Fix by moving the generic TypeParamInquiry operator() overload into the derived classes that do not define this overload themselves.

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

Diff Detail

Event Timeline

Meinersbur created this revision.Aug 22 2020, 4:12 PM
Herald added a project: Restricted Project. · View Herald Transcript
Meinersbur requested review of this revision.Aug 22 2020, 4:12 PM
Meinersbur retitled this revision from [flang][msvc] Remove ambiguous overload. NFCI. to [flang][msvc] Avoid ambiguous overload from base class. NFC..
Meinersbur edited the summary of this revision. (Show Details)

Considering more occurrences in check-expressions.cpp

Meinersbur edited the summary of this revision. (Show Details)Aug 23 2020, 12:15 PM

Fix missing helpers

The expression traversal framework in traverse.h depends on the C++17 compiler implementing base class overloads correctly; the traversal class implements a no-op visitor, and clients override the framework's no-op member functions for expression object types of interest. Please don't litter all of its clients with no-op overrides that will live forever.

What is your alternative suggestion to fix the compiler error?

What is your alternative suggestion to fix the compiler error?

Is the MSVC bug peculiar to template member functions like template <int K> operator()(const TypeParamInquiry<K> &) const? If so, is it peculiar to *just* TypeParamInquiry, or does it affect other templatized expression nodes as well? If that turns out to be the case, there's some changes to the expression representation that could be tried that would avoid the use of a template in that case.

Another possible work-around, if the problem is specific to this one expression representation class: specialize the member function in the general Traverse class for each of the integer kinds.

Is the MSVC bug peculiar to template member functions like template <int K> operator()(const TypeParamInquiry<K> &) const? If so, is it peculiar to *just* TypeParamInquiry, or does it affect other templatized expression nodes as well?

The error happens with hidden methods with non-type templated method in templated base classes. The smallest reproducer I could create is this: https://godbolt.org/z/59ar5d. It does not occur when the template parameter is a class type: https://godbolt.org/z/1xo8jh. Thus, I think TypeParamInquiry is the only one affected.

If that turns out to be the case, there's some changes to the expression representation that could be tried that would avoid the use of a template in that case.

Could you elaborate?

Another possible work-around, if the problem is specific to this one expression representation class: specialize the member function in the general Traverse class for each of the integer kinds.

Which specializations for KIND do we need? 8,16,32,64, any other?

Is the MSVC bug peculiar to template member functions like template <int K> operator()(const TypeParamInquiry<K> &) const? If so, is it peculiar to *just* TypeParamInquiry, or does it affect other templatized expression nodes as well?

The error happens with hidden methods with non-type templated method in templated base classes. The smallest reproducer I could create is this: https://godbolt.org/z/59ar5d. It does not occur when the template parameter is a class type: https://godbolt.org/z/1xo8jh. Thus, I think TypeParamInquiry is the only one affected.

If that turns out to be the case, there's some changes to the expression representation that could be tried that would avoid the use of a template in that case.

Could you elaborate?

Another possible work-around, if the problem is specific to this one expression representation class: specialize the member function in the general Traverse class for each of the integer kinds.

Which specializations for KIND do we need? 8,16,32,64, any other?

class TypeParameterInquiry could be redefined as an operation that always returned the same kind of INTEGER result, and explicit conversions to other kinds would be wrapped around its usage. It would become an operation allowed only in one instantiation of Expr<Type<TypeCategory::Integer, KIND>>. This change would be big enough to have some ripple effects, but I'd be happy to do it for you as an experiment that you could try with your MSVC environment.

The second work-around would use specializations for KIND values of 1, 2, 4, and 8.

Use explicit method specializations in base class.

This solution is also accepted by msvc. A specialization TypeParamInquiry<16> is needed as well, otherwise it does not compiler.

I am not familiar with TypeParameterInquiry. Since this solution works, a restructure might not be necessary unless you plan to do the change anyway.

I am not familiar with TypeParameterInquiry. Since this solution works, a restructure might not be necessary unless you plan to do the change anyway.

I will post a patch here shortly that you can try.

Here's a patch that makes TypeParamInquiry monomorphic. Ignore the test failures with extra conversions -- I'll clean those up if you prefer this approach.

I can confirm that shape.cpp compiles with your patch.

I'd prefer you patch, mostly because its reduced complexity. However, I think it's the code owner to decide which version they prefer.

I can confirm that shape.cpp compiles with your patch.

I'd prefer you patch, mostly because its reduced complexity. However, I think it's the code owner to decide which version they prefer.

Thanks for checking. Let me know how it works out with the code owner.

I have my alternative patch coming soon to phabricator, and I'll include you on its review.

According to CODE_OWNERS.TXT, that @sscalpone

@Meinersbur. Hi Michael, I believe you prefer @klausler to make the patch, so he's going ahead with it.

If this is now fixed in another way, can this review be closed?

Meinersbur abandoned this revision.Sep 1 2020, 3:58 PM

Yes, abandon in favor of D86551.