Page MenuHomePhabricator

Fix source range of defaulted/deleted member declarations
ClosedPublic

Authored by eliben on Mar 19 2015, 2:35 PM.

Details

Summary

Fixes https://llvm.org/bugs/show_bug.cgi?id=20744

struct A {

A() = default;

};

Previously the source range of the declaration of A ended at the ')'. It should include the '= default' part as well. The same for '= delete'.

Note: this will break one of the clang-tidy fixers, which is going to be addessed in a follow-up patch.

Diff Detail

Repository
rL LLVM

Event Timeline

eliben updated this revision to Diff 22303.Mar 19 2015, 2:35 PM
eliben retitled this revision from to Fix source range of defaulted/deleted member declarations.
eliben updated this object.
eliben edited the test plan for this revision. (Show Details)
eliben added reviewers: rnk, jpienaar.
eliben added a subscriber: Unknown Object (MLST).

Note: followup patch to clang-tidy is http://reviews.llvm.org/D8466

jpienaar edited edge metadata.Mar 20 2015, 9:21 AM

Looks good. Is it better to adjust the range here or n ParseCXXInlineMethodDef?

Looks good. Is it better to adjust the range here or n ParseCXXInlineMethodDef?

The problem with ParseCXXInlineMethodDef is that we no longer have access to these tokens in it. ParseCXXInlineMethodDef expects to see a { for a def or ; for a =default/=delete method. So we'd have to push DeleteOrDefaultEndLoc through to ParseCXXInlineMethodDef - it would result in an additional argument, more code and I'm not sure what gain.

jpienaar accepted this revision.Mar 20 2015, 10:24 AM
jpienaar edited edge metadata.

Looks good. Is it better to adjust the range here or n ParseCXXInlineMethodDef?

The problem with ParseCXXInlineMethodDef is that we no longer have access to these tokens in it. ParseCXXInlineMethodDef expects to see a { for a def or ; for a =default/=delete method. So we'd have to push DeleteOrDefaultEndLoc through to ParseCXXInlineMethodDef - it would result in an additional argument, more code and I'm not sure what gain.

OK, I thought KWLoc that is set in ParseCXXInlineMethodDef could be used to get it.

This revision is now accepted and ready to land.Mar 20 2015, 10:24 AM
rnk edited edge metadata.Mar 20 2015, 11:39 AM

It doesn't look too hard to do this locally in ParseCXXInlineMethodDef by saving the end of the current token location before calling TryConsumeToken:

SourceLocation KWEndLoc = Tok.getEndLoc();
if (TryConsumeToken(...)) {
  ...
  FnD->setRangeEnd(KWEndLoc);
} else if (TryConsumeToken(...)) {
  ...
  FnD->setRangeEnd(KWEndLoc);
}

Seem reasonable?

eliben updated this revision to Diff 22367.Mar 20 2015, 12:37 PM
eliben edited edge metadata.
In D8465#144236, @rnk wrote:

It doesn't look too hard to do this locally in ParseCXXInlineMethodDef by saving the end of the current token location before calling TryConsumeToken:

SourceLocation KWEndLoc = Tok.getEndLoc();
if (TryConsumeToken(...)) {
  ...
  FnD->setRangeEnd(KWEndLoc);
} else if (TryConsumeToken(...)) {
  ...
  FnD->setRangeEnd(KWEndLoc);
}

Seem reasonable?

Ah, indeed. This makes things simpler. PTAL

Thanks

Looks good. Is it better to adjust the range here or n ParseCXXInlineMethodDef?

The problem with ParseCXXInlineMethodDef is that we no longer have access to these tokens in it. ParseCXXInlineMethodDef expects to see a { for a def or ; for a =default/=delete method. So we'd have to push DeleteOrDefaultEndLoc through to ParseCXXInlineMethodDef - it would result in an additional argument, more code and I'm not sure what gain.

OK, I thought KWLoc that is set in ParseCXXInlineMethodDef could be used to get it.

Yes, indeed. Updated the patch.

Thanks

eliben updated this revision to Diff 22368.Mar 20 2015, 12:40 PM

whitespace cleanup

rnk added inline comments.Mar 23 2015, 10:51 AM
lib/Parse/ParseCXXInlineMethods.cpp
73 ↗(On Diff #22368)

Any reason we can't save Tok.getEndLoc() here? Then we don't have to hardcode the lengths of delete and default.

80 ↗(On Diff #22368)

This doesn't do the right thing for function templates, which the only case where this dyn_cast can fail. I think you can write a templated constructor and default it. =P A robust way to do this might be:

auto *FD = dyn_cast<FunctionDecl>(FnD);
FD = FD ? FD : cast<FunctionTemplateDecl>(FnD)->getTemplatedDecl();
FD->setRangeEnd(KWEndLoc);
eliben added inline comments.Mar 23 2015, 12:43 PM
lib/Parse/ParseCXXInlineMethods.cpp
73 ↗(On Diff #22368)

Great suggestion, thanks. Done

80 ↗(On Diff #22368)

Reid, I may be missing something here, do you mean something like:

struct Foot {

template <typename T>
Foot &operator=(const T& Other) = default;

};

I don't think this is actually valid. The standard says in 8.4.2:

"A function that is explicitly defaulted shall ... have the same declared function type (except for possibly differing ref-qualifiers and except that in the case of a copy constructor or copy assignment operator, the parameter type may be “reference to non-const T”, where T is the name of the member function’s class) as if it had been implicitly declared,
and"

And indeed Clang complains:

error: only special member functions may be defaulted
   Foot &operator=(const T& Other) = default;
eliben updated this revision to Diff 22502.Mar 23 2015, 12:43 PM

Using Tok.getEndLoc(). to avoid recomputing

rnk accepted this revision.Mar 23 2015, 2:26 PM
rnk edited edge metadata.

lgtm

This revision was automatically updated to reflect the committed changes.

r233028

Thanks for the review