Page MenuHomePhabricator

[lldb] Don't reject empty or unnamed template parameter packs
ClosedPublic

Authored by teemperor on Dec 1 2020, 1:30 PM.

Details

Summary

We currently reject all templates that have either zero args or that have a parameter pack without
a name. Both cases are actually allowed in C++, so rejecting them leads to LLDB instead falling back to
a dummy 'void' type. This leads to all kind of errors later on (most notable, variables that have such
template types appear to be missing as we can't have 'void' variables and inheriting from such a template
type will cause Clang to hit some asserts when finding that the base class is 'void').

This just removes the too strict tests and adds a few tests for this stuff (+ some combinations of these
tests with preceding template parameters).

Things that I left for follow-up patches:

  • All the possible interactions with template-template arguments which seem like a whole new source of possible bugs.
  • Function templates which completely lack sanity checks.
  • Variable templates are not implemented.
  • Alias templates are not implemented too.
  • The rather strange checks that just make sure that the separate list of template arg names and values always have the same length. I believe those ought to be asserts, but my current plan is to move both those things into a single list that can't end up in this inconsistent state.

Diff Detail

Event Timeline

teemperor created this revision.Dec 1 2020, 1:30 PM
teemperor requested review of this revision.Dec 1 2020, 1:30 PM
shafik added a comment.Dec 1 2020, 3:33 PM

There is also alias templates:

template<typename...>
using void_t = void;

might as well throw that in.

shafik accepted this revision.Dec 1 2020, 3:33 PM

LGTM

This revision is now accepted and ready to land.Dec 1 2020, 3:33 PM
JDevlieghere accepted this revision.Dec 1 2020, 8:47 PM

There seems to be quite a bit of repetition in the test, maybe a little helper would ameliorate that. Otherwise LGTM.

teemperor edited the summary of this revision. (Show Details)Dec 2 2020, 1:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2020, 1:51 AM

This caused failures on the Windows lldb buildbot:

http://lab.llvm.org:8011/#/builders/83/builds/1294

Tests have to have unique names, so the two new added tests that use TestCase as the class name are both trying to write to the same log file and failing to do so.

This caused failures on the Windows lldb buildbot:

http://lab.llvm.org:8011/#/builders/83/builds/1294

Tests have to have unique names, so the two new added tests that use TestCase as the class name are both trying to write to the same log file and failing to do so.

Thanks, fixed.

FWIW, this (frankly silly) requirement is actually supposed to be fixed by D83767.

This caused failures on the Windows lldb buildbot:

http://lab.llvm.org:8011/#/builders/83/builds/1294

Tests have to have unique names, so the two new added tests that use TestCase as the class name are both trying to write to the same log file and failing to do so.

Thanks, fixed.

FWIW, this (frankly silly) requirement is actually supposed to be fixed by D83767.

It would be great to finally have it fixed so don't have random failures because of file names!

It looks like the two tests are still failing though (different reason):

http://lab.llvm.org:8011/#/builders/83/builds/1332/steps/7/logs/stdio