Page MenuHomePhabricator

Fix assertion failure on MaybeODRUseExprs.

Authored by manmanren on Feb 24 2016, 10:49 AM.



In VisitNonTypeTemplateParamDecl, before SubstExpr with the default argument,
we should create a ConstantEvaluated ExpressionEvaluationContext. Without this,
it is possible to use a PotentiallyEvaluated ExpressionEvaluationContext; and
MaybeODRUseExprs will not be cleared when popping the context, causing
assertion failure.

This is similar to how we handle the context before SubstExpr with the
default argument, in SubstDefaultTemplateArgument.

Part of PR13986.

Diff Detail


Event Timeline

manmanren updated this revision to Diff 48963.Feb 24 2016, 10:49 AM
manmanren retitled this revision from to Fix assertion failure on MaybeODRUseExprs..
manmanren updated this object.
manmanren added reviewers: rsmith, EricWF, faisalv.
manmanren added a subscriber: cfe-commits.
faisalv added inline comments.Feb 24 2016, 12:14 PM
2114 ↗(On Diff #48963)

Looks reasonable to me - since it is consistent with the other changes I had made. But somewhat orthogonal to your fix, I wouldn't mind Richard commenting on why constant expression evaluation does not have IsPotentiallyEvaluatedContext return false (and avoid adding an entry to MaybeODRUse)
Also, just as SubstituteDefaultTemplateArgument does, Perhaps we should add an InstantiatingTemplate on the stack that marks this as a substitution into default arguments. It would be nice if we could refactor both into a call to the same function (i.e SubstittueDefaultTempalteArgument) - but looking at the code for it, that would be a little tricky.
My 2 cents.

rsmith accepted this revision.Feb 24 2016, 2:01 PM
rsmith edited edge metadata.
rsmith added inline comments.
2114 ↗(On Diff #48963)

"Potentially evaluated" is a term defined in the standard; it would be confusing for us to use it to mean something else. Constant expressions *are* evaluated (typically only during compilation), and they can result in odr-use. The cases where they don't do so are somewhat specialized.

I don't think we need or want a separate entry on the instantiation stack, as we're still instantiating pieces of the same entity, with the same instantiation semantics.

This revision is now accepted and ready to land.Feb 24 2016, 2:01 PM
This revision was automatically updated to reflect the committed changes.