Page MenuHomePhabricator

DO NOT SUBMIT: workaround for context-sensitive use of non-type-template-parameter integer suffixes
ClosedPublic

Authored by dblaikie on Oct 8 2021, 5:45 PM.

Details

Summary

There's a nuanced check about when to use suffixes on these integer
non-type-template-parameters, but when rebuilding names for
-gsimple-template-names there isn't enough data in the DWARF to
determine when to use suffixes or not. So turn on suffixes always to
make it easy to match up names in llvm-dwarfdump --verify.

I /think/ if we correctly modelled auto non-type-template parameters
maybe we could put suffixes only on those. But there's also some logic
in Clang that puts the suffixes on overloaded functions - at least
that's what the parameter says (see D77598 and printTemplateArguments
"TemplOverloaded" parameter) - but I think maybe it's for anything that
/can/ be overloaded, not necessarily only the things that are overloaded
(the argument value is hardcoded at the various callsites, doesn't seem
to depend on overload resolution/searching for overloaded functions). So
maybe with "auto" modeled more accurately, and differentiating between
function templates (always using type suffixes there) and class/variable
templates (only using the suffix for "auto" types) we could correctly
use integer type suffixes only in the minimal set of cases.

But maybe that's all too much fuss and we could/should just put them
everywhere always?

(more context:

Diff Detail

Event Timeline

dblaikie requested review of this revision.Oct 8 2021, 5:45 PM
dblaikie created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2021, 5:45 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aprantl accepted this revision.Oct 29 2021, 11:01 AM
aprantl added a reviewer: shafik.

I think this is fine.

This revision is now accepted and ready to land.Oct 29 2021, 11:01 AM
This revision was landed with ongoing or failed builds.Mon, Nov 1, 5:09 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMon, Nov 1, 5:09 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
shafik added a comment.Mon, Nov 1, 6:12 PM

I think this is fine as well.

ldionne added a subscriber: ldionne.Wed, Nov 3, 8:22 AM

This broke libc++'s CI: https://buildkite.com/llvm-project/libcxx-ci/builds/6374#7569da95-c852-44f9-8b69-947245cf0b65

When you make a change to Clang AND the libc++ tests at the same time, you have to account for the fact that we support older versions of Clang. For example, here, you changed the pretty printer test, but we still run that test with Clang 12 and Clang 13, not only with Clang main. Since older Clangs don't contain your change, the test fails (and rightfully so). I'll add the necessary XFAILs, but please look out for these breakages since they are somewhat disruptive. Thanks!

This broke libc++'s CI: https://buildkite.com/llvm-project/libcxx-ci/builds/6374#7569da95-c852-44f9-8b69-947245cf0b65

When you make a change to Clang AND the libc++ tests at the same time, you have to account for the fact that we support older versions of Clang. For example, here, you changed the pretty printer test, but we still run that test with Clang 12 and Clang 13, not only with Clang main. Since older Clangs don't contain your change, the test fails (and rightfully so). I'll add the necessary XFAILs, but please look out for these breakages since they are somewhat disruptive. Thanks!

Thanks for the context/fix - I'll try to keep it in mind.

Some offline post-commit feedback from @rsmith - Rename printing policy to match closer to the function it's tested in. Will think about that, check the function name, etc, and commit a patch in the next few days, hopefully.

Renamed in 6512098877c3a230bbd38bc81b14a4e5844739ff to AlwaysIncludeTypeForTemplateArgument