This is an archive of the discontinued LLVM Phabricator instance.

Fix implicit copy ctor and copy assignment operator warnings when -Wdeprecated passed.
ClosedPublic

Authored by hintonda on Mar 12 2016, 3:42 PM.

Diff Detail

Event Timeline

hintonda updated this revision to Diff 50529.Mar 12 2016, 3:42 PM
hintonda retitled this revision from to Fix implicit copy ctor and copy assignment operator warnings when -Wdeprecated passed..
hintonda updated this object.
hintonda added a reviewer: rjmccall.
hintonda added a subscriber: cfe-commits.
rjmccall edited edge metadata.Mar 13 2016, 12:47 PM

It has side effects in the destructor. It's not copyable.

It could reasonably be made movable, however, now that we require C++11 as a host requirement. (It was written before that was true.)

LookupResult's copy ctor and assignment operator are used in Sema::LookupInlineAsmField(). Looks like these were added back in December.

I'll remove the defaults, from this patch, but not sure how to handle LookupInlineAsmField(). Will add author of that change.

OTOH, UnresolvedSetImpl requires an assignment operator since it's a base to UnresolvedSet which has an implicit assignment operator, so I think that change is correct.

myatsina edited edge metadata.Mar 14 2016, 2:09 AM

LookupResult's copy ctor and assignment operator are used in Sema::LookupInlineAsmField(). Looks like these were added back in December.

I'll remove the defaults, from this patch, but not sure how to handle LookupInlineAsmField(). Will add author of that change.

OTOH, UnresolvedSetImpl requires an assignment operator since it's a base to UnresolvedSet which has an implicit assignment operator, so I think that change is correct.

I can change my code to avoid the copy ctor and assignment operator of LookupResult - In every iteration I only need to access 2 members of the previous LookupResult (isSingleResult() and getFoundDecl()), I can keep only them instead of the whole object.
Do you want me to upload a patch with this change?

Sure, that would be great. Thanks.

hintonda updated this revision to Diff 50611.EditedMar 14 2016, 10:13 AM
hintonda edited edge metadata.

Add move ctor and assignment operator defaults to match
UnresolvedSetImpl's contained SmallVector.

Add FIXME note to LookupResult. Will address FIXME once Marina's
pending change to Sema::LookupInlineAsmField() is available.

Btw, what do you think about making -Wdeprecated the default for llvm/clang builds?

With this change, llvm/clang will be clean. However, libcxx and libcxxabi have a bunch of throw warnings.

UNresolvedSetImpl isn't copy constructible, so should it really be copy
assignable? (it looks like it only needs to be so because of LookupResult -
so once LookupResult is fixed, UnresolvedSetImpl can go back to the way it
was)

Perhaps we should just wait for the fix for LookupResult copying in
SemaStmtAsm from Marina?

Are you planning to cleanup -Wdeprecated in libcxx and libcxxabi too?

I did most of the -Wdeprecated cleanup late last year - but when I tried to
turn it on I hit issues in various stdlibs, etc. It'd be great to finish
the cleanup, or suppress it in system headers, etc, and get it turned on.

I've uploaded the following review:
http://reviews.llvm.org/D18175

Making UnresolvedSet copyable / movable seems reasonable to me.

I've committed the fix in revision 263630

hintonda updated this revision to Diff 50823.Mar 16 2016, 7:54 AM

Address FIXME now that Sema::LookupInlineAsmField() has been fixed.

hintonda updated this revision to Diff 50843.Mar 16 2016, 11:38 AM

Added FIXME comment, and reformated with clang-format.

hintonda updated this revision to Diff 50930.Mar 17 2016, 7:24 AM

Fixed comment per Dave's recommendation.

This revision was automatically updated to reflect the committed changes.