This is an archive of the discontinued LLVM Phabricator instance.

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

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–74

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

81

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–74

Great suggestion, thanks. Done

81

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