This is an archive of the discontinued LLVM Phabricator instance.

In MSVC compatibility mode, handle unqualified templated base class initialization
ClosedPublic

Authored by frederic-tingaud-sonarsource on Apr 29 2022, 3:26 AM.

Details

Summary

Before C++20, MSVC was supporting not mentioning the template argument of the base class when initializing a class inheriting a templated base class.
So the following code compiled correctly:

template <class T>
class Base {
};

template <class T>
class Derived : public Base<T> {
public:
  Derived() : Base() {}
};

void test() {
    Derived<int> d;
}

See https://godbolt.org/z/Pxxe7nccx for a conformance view.

This patch adds support for such construct when in MSVC compatibility mode.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2022, 3:26 AM
frederic-tingaud-sonarsource requested review of this revision.Apr 29 2022, 3:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2022, 3:26 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rnk requested changes to this revision.May 3 2022, 9:35 AM
rnk added inline comments.
clang/lib/Sema/SemaDeclCXX.cpp
4310–4311

We have to make sure our MS compatibility logic fires *before* we try to correct typos. It's currently a big issue that we don't. You can try adjusting your test case by adding a similarly named class like Baze and see if you still get the desired behavior.

See this old issue from 2014:
https://github.com/llvm/llvm-project/issues/20623

This revision now requires changes to proceed.May 3 2022, 9:35 AM

Added two similar names in the tests, to try to trick the typo correction.

rnk added inline comments.May 3 2022, 3:59 PM
clang/lib/Sema/SemaDeclCXX.cpp
4310–4311

What I'd like to see is just to move the MSVCCompat check up before the typo correction. Basically, once we start typo correction, we *must* emit an error diagnostic. We may attempt to recover by returning a recovery AST node, but the compilation should fail overall.

I think in practice, we only do typo correction if lookup is empty, and your code only fires if name lookup returns a single result, so it's functionally equivalent to reorder these codepaths. I'd prefer that structure, mostly for readability.

Sorry about the confusion, it makes sense. Fixed.

By the way, in the current state of the code, there is no risk of such conflict between typo correction and UnqualifiedBase, because when an unqualified base exists the lookup result will not be empty.
That's why the test was not failing even without moving the compatibility first.

rnk accepted this revision.May 4 2022, 11:37 AM

lgtm, thanks!

By the way, in the current state of the code, there is no risk of such conflict between typo correction and UnqualifiedBase, because when an unqualified base exists the lookup result will not be empty.
That's why the test was not failing even without moving the compatibility first.

Yep, that's exactly it, but with the reordering it's somewhat clearer that, if we make this work for MS compat purposes, typo correction will not occur.

This revision is now accepted and ready to land.May 4 2022, 11:37 AM