Page MenuHomePhabricator

Make function local tags visible.
ClosedPublic

Authored by v.g.vassilev on Aug 14 2016, 1:59 PM.

Details

Reviewers
rsmith
Summary

Upon instantiating function decls, we should mark visible any tag decl in the function's body.

Diff Detail

Event Timeline

v.g.vassilev retitled this revision from to Make function local tags visible..
v.g.vassilev updated this object.
v.g.vassilev added a reviewer: rsmith.
v.g.vassilev set the repository for this revision to rL LLVM.
v.g.vassilev added a subscriber: cfe-commits.
rsmith added inline comments.Aug 15 2016, 11:24 AM
lib/Sema/SemaLookup.cpp
1546–1548 ↗(On Diff #67988)

This check should already do the right thing for the case of a function-scope declaration.

test/Modules/Inputs/PR28794/Subdir/LibBHeader.h
7–11

We should not be instantiating this template in the first place: the template definition should not be visible within pr28794.cpp because the LibBHeader.h submodule is never imported. I would guess the bug is that we're missing a visibility check for the template definition when we trigger the instantiation here.

v.g.vassilev removed rL LLVM as the repository for this revision.

Do not instantiate function declarations which are not visible.

Add forgotten change.

rsmith added inline comments.Aug 17 2016, 2:17 PM
lib/Sema/SemaTemplateInstantiateDecl.cpp
3596–3597

Please update this FIXME to just "We need to track the instantiation stack in order to know which definitions should be visible within this instantiation."

3597–3602

I think this should be checked before we deal with late-parsed templates -- if the template definition isn't visible, an attempt to instantiate it shouldn't trigger it being parsed.

rsmith added inline comments.Aug 17 2016, 2:21 PM
lib/Sema/SemaTemplateInstantiateDecl.cpp
3598–3599

If you can do that, it would be preferable. You'll presumably need to change the PatternDecl argument to PatternDecl->isThisDeclarationADefinition() || PatternDecl->isDefaulted() ? PatternDecl : nullptr or similar.

3652–3655

Yes, let's do this. It's not quite right, but the same is true for the InstantiateClass and InstantiateEnum cases, and it makes sense to handle all those cases the same way.

v.g.vassilev added inline comments.Aug 18 2016, 4:36 AM
lib/Sema/SemaTemplateInstantiateDecl.cpp
3597–3602

If I am to sink the diags under the // TODO: this might change behavior. The diagnostics may not kick-in properly in case of late template parsing (see around line 3594).

v.g.vassilev marked 2 inline comments as done.

Add setHidden(false) and update fixme.

rsmith added inline comments.Aug 18 2016, 2:09 PM
lib/Sema/SemaTemplateInstantiateDecl.cpp
3597–3602

As long as we treat a late-parsed template as being defined, it should be fine (and looking at FunctionDecl::isThisDeclarationADefinition, we do). It's not correct to parse a late-parsed template that's not visible, so we need to move this check to before we handle late-parsed templates for correctness.

v.g.vassilev marked an inline comment as done.

Move DiagnoseUninstantiableTemplate before the late template parsing.

rsmith accepted this revision.Aug 18 2016, 3:06 PM
rsmith edited edge metadata.
This revision is now accepted and ready to land.Aug 18 2016, 3:06 PM
v.g.vassilev edited edge metadata.
v.g.vassilev marked an inline comment as done.

Sink some of the diagnostics in the DiagnoseUninstantiableTemplate.

akuegel added inline comments.
lib/Sema/SemaTemplateInstantiateDecl.cpp
3617

Line 3619 to 3623 breaks the compilation of some of our targets. I am not sure if it is because of a mistake in our code or if there is something bad in this patch.
This comment seems to indicate that there is still something missing to make the check here correct?

v.g.vassilev added inline comments.Aug 22 2016, 9:54 AM
lib/Sema/SemaTemplateInstantiateDecl.cpp
3617

@akuegel, would you be able to reduce a standalone example?

akuegel added inline comments.Aug 22 2016, 10:14 AM
lib/Sema/SemaTemplateInstantiateDecl.cpp
3617

I tried to create a simple example, but those simple cases worked fine. So I am not sure what is going on in the more complex cases. I have asked Richard for help. Since Richard has access to that broken code, I hope he will figure it out.

v.g.vassilev added inline comments.Aug 22 2016, 10:35 AM
lib/Sema/SemaTemplateInstantiateDecl.cpp
3617

Thanks! Otherwise, maybe creduce could help.

Should be fixed in r279486, which also incorporates the updates to this change since it landed (sinking diagnostics into DiagnoseUninstantiableTemplate).

lib/Sema/SemaTemplate.cpp
494–495

} and else on the same line please. (Here and in a couple of places below.)

499–501

Sink this into the err_template_instantiate_undefined case below.

503–505

Sink this into the InstantiatedFromMember case below.

Thanks! Shall I close this?

v.g.vassilev closed this revision.Aug 30 2016, 10:45 AM

Landed in r279164 and r279486.