Page MenuHomePhabricator

Modify DeclaratorChuck::getFunction to be passed an Exception Specification SourceRange

Authored by nwilson on Aug 5 2015, 4:17 PM.


  • Store the exception specification range's begin and end SourceLocation in DeclaratorChuck::FunctionTypeInfo. These SourceLocations can be used in a FixItHint Range.
  • Add diagnostic; function concept having an exception specification.

Diff Detail


Event Timeline

nwilson updated this revision to Diff 31414.Aug 5 2015, 4:17 PM
nwilson retitled this revision from to Modify DeclaratorChuck::getFunction to be passed an Exception Specification SourceRange.
nwilson updated this object.
nwilson added a subscriber: cfe-commits.
rsmith added inline comments.Aug 5 2015, 5:45 PM
1977 ↗(On Diff #31414)

def err -> def err
can not -> cannot
specifiers -> specification

1330–1336 ↗(On Diff #31414)

Please add a getExceptionSpecRange function returning the SourceRange...

7447–7450 ↗(On Diff #31414)

This will assert if there isn't a FunctionTypeInfo for the declaration, which can theoretically happen if it's declared via an (ill-formed today) typedef. (It also might not provide a source range if the exception specification is implicit, for instance because the function template is a destructor or deallocation function, but passing an empty SourceRange to the FixItHint should just result it in being ignored.)

7449–7452 ↗(On Diff #31414)

... and use it here.

7454–7455 ↗(On Diff #31414)

No newline before else.

7456 ↗(On Diff #31414)

Add a comment here indicating why we're doing this. Maybe a snippet of the relevant rule from the TS?

7456–7458 ↗(On Diff #31414)

Use ASTContext::adjustExceptionSpec(NewFD, EST_BasicNoexcept) for this.

7456–7458 ↗(On Diff #31414)

You don't seem to have test coverage for this part. Maybe test it with static_assert(noexcept(SomeConcept<T>()));?

I'll make the fixes based on the comments, but had question about the FunctionTypeInfo in the meantime.

7447–7450 ↗(On Diff #31414)

Hmm, I'm not sure if we'd run into that case because I don't believe we can have a concept specified as a typedef (check is yet to be added) and a check exists for being in a non-namespace scope. Do you think a check should still be added verifying that the FunctionTypeInfo exists?

rsmith added inline comments.Aug 5 2015, 8:32 PM
7447–7450 ↗(On Diff #31414)

Yes, you should check that a FunctionTypeInfo exists (isFunctionDeclarator) rather than assuming that it does. The testcase would look something like this:

typedef int Fn() noexcept;
template<typename T> concept Fn C;

I think we'll discard the noexcept early at the moment, but that's not something you should be subtly relying on here (especially given that is looming on the horizon). Something like:

SourceRange Range;
if (D.isFunctionDeclarator())
  Range = D.getFunctionTypeInfo().getExceptionSpecRange();

... should handle this safely.

nwilson updated this revision to Diff 31488.Aug 6 2015, 3:54 PM

Updated Patch based on comments; fix diagnostic spacing and phrasing, add getExceptionSpecRange, check FunctionTypeInfo exists, use PartialDiagnostic, add static_assert test for function concept being treated as noexcept(true)

rsmith added inline comments.Aug 6 2015, 4:23 PM
1262 ↗(On Diff #31488)

Drop bogus "keyword introducing the" here, and drop either the same words from the previous comment or the word "beginning".

7450 ↗(On Diff #31488)

The D.isFunctionDeclarator() check should be nested within this, as should the Range variable:

if (FPT->hasExceptionSpec()) {
  SourceRange Range;
  if (D.isFunctionDeclarator())
    Range = D.getFunctionTypeInfo().getExceptionSpecRange();
  PartialDiagnostic PD = ...
7454–7456 ↗(On Diff #31488)

You don't need this if; a FixItHint with an invalid range has no effect.

9–10 ↗(On Diff #31488)

This isn't quite enough; noexcept will also return true if the expression is a constant expression. Maybe replace the return true with return (throw 0, true).

nwilson added inline comments.Aug 6 2015, 7:09 PM
7454–7456 ↗(On Diff #31488)

Ahhh, okay. I misunderstood the original comment about an empty SourceRange apparently.

I'll go back to using Diagnostic rather than PartialDiagnostic as well.

nwilson updated this revision to Diff 31502.Aug 6 2015, 7:25 PM
  • Update Patch based on comments; rephrase exception spec comment, adjust nesting of check for FunctionTypeInfo, remove SourceRange check, fix test
nwilson added inline comments.Aug 6 2015, 7:26 PM
1262 ↗(On Diff #31502)

Please let me know if there is still an issue with the wording here.

rsmith added inline comments.Aug 14 2015, 4:49 PM
1262 ↗(On Diff #31502)

It'd be better to use "specification" rather than "specifier" here to match the standard.

7453–7455 ↗(On Diff #31502)

Move this code out of the if; we should issue an error whether or not the function was spelled with a function declarator.

9 ↗(On Diff #31502)

I guess this should be either throw 0, true or throw, true.

nwilson updated this revision to Diff 32228.Aug 15 2015, 9:23 PM

Patch addressing comments; fix comment/documentation wording, scoping of diagnostic and setting invalid declaration, and fix test.

I also modified the location of the diagnostic indicating an exception specification is not allowed to point to the function declarator in order to handle a range having an invalid begin location.

The 80 column limit was overrun in SemaType.cpp, so clang format has been run on the problematic lines in the file.

rsmith accepted this revision.Aug 25 2015, 3:03 PM
rsmith edited edge metadata.

LGTM, sorry for the delay.

This revision is now accepted and ready to land.Aug 25 2015, 3:03 PM
nwilson closed this revision.Aug 25 2015, 9:20 PM
This revision was automatically updated to reflect the committed changes.