[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: Re: [PATCH] D11334: [Sema] Call to deleted functions are supposed to be verboten
From: Richard Smith <richard () metafoo ! co ! uk>
Date: 2015-07-21 22:58:38
Message-ID: CAOfiQqkHw4r-zQMO+eqz0QDz7f8-jtuCq7X=Kv_rWbD+y6yY0A () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Tue, Jul 21, 2015 at 3:39 PM, Davide Italiano <dccitaliano@gmail.com>
wrote:
> On Tue, Jul 21, 2015 at 3:26 PM, Davide Italiano <dccitaliano@gmail.com>
> wrote:
> > On Tue, Jul 21, 2015 at 2:59 PM, Richard Smith <richard@metafoo.co.uk>
> wrote:
> >> rsmith added inline comments.
> >>
> >> ================
> >> Comment at: lib/Sema/SemaOverload.cpp:11608
> >> @@ +11607,3 @@
> >> +
> >> + // Calls to deleted member functions are verboten.
> >> + if (Method && Method->isDeleted())
> >> ----------------
> >> This check should happen when we build the `MemberExpr`, not when we
> use it. It looks like the bug is in `Sema::BuildMemberReferenceExpr`, at
> around line 1050 of SemaExprMember.cpp:
> >>
> >> bool ShouldCheckUse = true;
> >> if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(MemberDecl)) {
> >> // Don't diagnose the use of a virtual member function unless it's
> >> // explicitly qualified.
> >> if (MD->isVirtual() && !SS.isSet())
> >> ShouldCheckUse = false;
> >> }
> >>
> >> // Check the use of this member.
> >> if (ShouldCheckUse && DiagnoseUseOfDecl(MemberDecl, MemberLoc))
> >> return ExprError();
> >>
> >> This is wrong: we should `DiagnoseUseOfDecl` (including diagnosing the
> use of a deleted function) even for a virtual function. Which tests fail if
> we unconditionally `DiagnoseUseOfDecl` here?
> >>
> >
> > Luckily only one.
> >
> > FAIL: Clang :: SemaCXX/attr-deprecated.cpp (5733 of 8388)
> > ******************** TEST 'Clang :: SemaCXX/attr-deprecated.cpp'
> > FAILED ********************
> > Script:
> > --
> > /exps/llvm2/build/./bin/clang -cc1 -internal-isystem
> > /exps/llvm2/build/bin/../lib/clang/3.8.0/include -nostdsysteminc
> > /exps/llvm2/tools/clang/test/SemaCXX/attr-deprecated.cpp -verify
> > -fexceptions
> > --
> > Exit Code: 1
> >
> > Command Output (stderr):
> > --
> > error: 'warning' diagnostics seen but not expected:
> > File /exps/llvm2/tools/clang/test/SemaCXX/attr-deprecated.cpp Line
> > 34: 'f' is deprecated
> > File /exps/llvm2/tools/clang/test/SemaCXX/attr-deprecated.cpp Line
> > 50: 'f' is deprecated
> > File /exps/llvm2/tools/clang/test/SemaCXX/attr-deprecated.cpp Line
> > 65: 'f' is deprecated
> > error: 'note' diagnostics seen but not expected:
> > File /exps/llvm2/tools/clang/test/SemaCXX/attr-deprecated.cpp Line
> > 29: 'f' has been explicitly marked deprecated here
> > File /exps/llvm2/tools/clang/test/SemaCXX/attr-deprecated.cpp Line
> > 29: 'f' has been explicitly marked deprecated here
> > File /exps/llvm2/tools/clang/test/SemaCXX/attr-deprecated.cpp Line
> > 62: 'f' has been explicitly marked deprecated here
> > 6 errors generated.
> >
> > --
>
> Looking at the tests it seems we emit the warnings we previously
> emitted only for fully qualified functions also for non-fully
> qualified functions.
> If you think we should diagnose in both cases, I'll modify the test to
> take them in account.
Yes, our current behavior is inconsistent, different from GCC, and
surprising. Let's diagnose in both cases.
[Attachment #5 (text/html)]
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jul 21, 2015 \
at 3:39 PM, Davide Italiano <span dir="ltr"><<a \
href="mailto:dccitaliano@gmail.com" \
target="_blank">dccitaliano@gmail.com</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Jul 21, 2015 at \
3:26 PM, Davide Italiano <<a \
href="mailto:dccitaliano@gmail.com">dccitaliano@gmail.com</a>> wrote:<br> > On \
Tue, Jul 21, 2015 at 2:59 PM, Richard Smith <<a \
href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br> >> \
rsmith added inline comments.<br> >><br>
>> ================<br>
>> Comment at: lib/Sema/SemaOverload.cpp:11608<br>
>> @@ +11607,3 @@<br>
>> +<br>
>> + // Calls to deleted member functions are verboten.<br>
>> + if (Method && Method->isDeleted())<br>
>> ----------------<br>
>> This check should happen when we build the `MemberExpr`, not when we use it. \
It looks like the bug is in `Sema::BuildMemberReferenceExpr`, at around line 1050 of \
SemaExprMember.cpp:<br> >><br>
>> bool ShouldCheckUse = true;<br>
>> if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(MemberDecl)) {<br>
>> // Don't diagnose the use of a virtual member function unless \
it's<br> >> // explicitly qualified.<br>
>> if (MD->isVirtual() && !SS.isSet())<br>
>> ShouldCheckUse = false;<br>
>> }<br>
>><br>
>> // Check the use of this member.<br>
>> if (ShouldCheckUse && DiagnoseUseOfDecl(MemberDecl, \
MemberLoc))<br> >> return ExprError();<br>
>><br>
>> This is wrong: we should `DiagnoseUseOfDecl` (including diagnosing the use \
of a deleted function) even for a virtual function. Which tests fail if we \
unconditionally `DiagnoseUseOfDecl` here?<br> >><br>
><br>
> Luckily only one.<br>
><br>
> FAIL: Clang :: SemaCXX/attr-deprecated.cpp (5733 of 8388)<br>
> ******************** TEST 'Clang :: SemaCXX/attr-deprecated.cpp'<br>
> FAILED ********************<br>
> Script:<br>
> --<br>
> /exps/llvm2/build/./bin/clang -cc1 -internal-isystem<br>
> /exps/llvm2/build/bin/../lib/clang/3.8.0/include -nostdsysteminc<br>
> /exps/llvm2/tools/clang/test/SemaCXX/attr-deprecated.cpp -verify<br>
> -fexceptions<br>
> --<br>
> Exit Code: 1<br>
><br>
> Command Output (stderr):<br>
> --<br>
> error: 'warning' diagnostics seen but not expected:<br>
> File /exps/llvm2/tools/clang/test/SemaCXX/attr-deprecated.cpp Line<br>
> 34: 'f' is deprecated<br>
> File /exps/llvm2/tools/clang/test/SemaCXX/attr-deprecated.cpp Line<br>
> 50: 'f' is deprecated<br>
> File /exps/llvm2/tools/clang/test/SemaCXX/attr-deprecated.cpp Line<br>
> 65: 'f' is deprecated<br>
> error: 'note' diagnostics seen but not expected:<br>
> File /exps/llvm2/tools/clang/test/SemaCXX/attr-deprecated.cpp Line<br>
> 29: 'f' has been explicitly marked deprecated here<br>
> File /exps/llvm2/tools/clang/test/SemaCXX/attr-deprecated.cpp Line<br>
> 29: 'f' has been explicitly marked deprecated here<br>
> File /exps/llvm2/tools/clang/test/SemaCXX/attr-deprecated.cpp Line<br>
> 62: 'f' has been explicitly marked deprecated here<br>
> 6 errors generated.<br>
><br>
> --<br>
<br>
</div></div>Looking at the tests it seems we emit the warnings we previously<br>
emitted only for fully qualified functions also for non-fully<br>
qualified functions.<br>
If you think we should diagnose in both cases, I'll modify the test to<br>
take them in account.</blockquote><div><br></div><div>Yes, our current behavior is \
inconsistent, different from GCC, and surprising. Let's diagnose in both \
cases.</div></div></div></div>
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic