Re: r288689 - Fix stack-use-after-scope in CheckExplicitlyDefaultedMemberExceptionSpec

2016-12-07 Thread Alex L via cfe-commits 
On 6 December 2016 at 19:39, Vitaly Buka  wrote:

> Hi Alex,
>
>
>
> On Tue, Dec 6, 2016 at 11:14 AM Alex L  wrote:
>
> Hi Vitaly,
>
> I noticed that you posted this patch up for review at
> https://reviews.llvm.org/D27422, but then committed it instantly without
> waiting for approval (and you did the same for r288685 as well). Is there
> any particular reason why you did this? I think that you should've waited
> for approval before committing.
>
>
> We had broken build bots, so seem like this trivial change is better than
> reverting patches.
> No.3 of http://llvm.org/docs/DeveloperPolicy.html#code-reviews allows
> after commit review for changes like this.
>

Thanks for the clarification. Yes, the developer policy states that you can
commit small changes and get them reviewed after committing, so a commit
like this would've been perfect for a post-commit review. However, it also
says that "Specifically, once a patch is sent out for review, it needs an
explicit “looks good” before it is submitted". Please avoid submitting
patches for review if you know that you will commit them without waiting
for approval in the future.


>
>
> This patch is pretty small, and so it looks to me like it could have been
> reviewed after it was committed, but patches that get post-commit reviews
> shouldn't get explicit review requests like this one did.
>
>
> Sorry, probably I did the same few time before. I can't find exact details
> in the policy, but I assumed that was a reasonable approach.
> So what is the process for after commit review?
>

People usually read the commit logs and check which commits need
post-commit reviews, and then look at them. There's no need for an explicit
review request for commits like this.


>
>
>
> Alex
>
> On 5 December 2016 at 19:25, Vitaly Buka via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
> Author: vitalybuka
> Date: Mon Dec  5 13:25:00 2016
> New Revision: 288689
>
> URL: http://llvm.org/viewvc/llvm-project?rev=288689=rev
> Log:
> Fix stack-use-after-scope in CheckExplicitlyDefaultedMemberExceptionSpec
>
> Summary:
> Similar to r288685.
> getExceptionSpec returned structure with pointers to temporarily object
> created
> by computeImplicitExceptionSpec.
>
> Reviewers: rsmith
>
> Subscribers: aizatsky, cfe-commits
>
> Differential Revision: https://reviews.llvm.org/D27422
>
> Modified:
> cfe/trunk/lib/Sema/SemaDeclCXX.cpp
>
> Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/
> SemaDeclCXX.cpp?rev=288689=288688=288689=diff
> 
> ==
> --- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Mon Dec  5 13:25:00 2016
> @@ -6299,8 +6299,8 @@ void Sema::CheckExplicitlyDefaultedMembe
>CallingConv CC = Context.getDefaultCallingConvention(/*
> IsVariadic=*/false,
>
> /*IsCXXMethod=*/true);
>FunctionProtoType::ExtProtoInfo EPI(CC);
> -  EPI.ExceptionSpec = computeImplicitExceptionSpec(*this,
> MD->getLocation(), MD)
> -  .getExceptionSpec();
> +  auto IES = computeImplicitExceptionSpec(*this, MD->getLocation(), MD);
> +  EPI.ExceptionSpec = IES.getExceptionSpec();
>const FunctionProtoType *ImplicitType = cast(
>  Context.getFunctionType(Context.VoidTy, None, EPI));
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r288689 - Fix stack-use-after-scope in CheckExplicitlyDefaultedMemberExceptionSpec

2016-12-06 Thread Vitaly Buka via cfe-commits 
Hi Alex,



On Tue, Dec 6, 2016 at 11:14 AM Alex L  wrote:

Hi Vitaly,

I noticed that you posted this patch up for review at
https://reviews.llvm.org/D27422, but then committed it instantly without
waiting for approval (and you did the same for r288685 as well). Is there
any particular reason why you did this? I think that you should've waited
for approval before committing.


We had broken build bots, so seem like this trivial change is better than
reverting patches.
No.3 of http://llvm.org/docs/DeveloperPolicy.html#code-reviews allows after
commit review for changes like this.


This patch is pretty small, and so it looks to me like it could have been
reviewed after it was committed, but patches that get post-commit reviews
shouldn't get explicit review requests like this one did.


Sorry, probably I did the same few time before. I can't find exact details
in the policy, but I assumed that was a reasonable approach.
So what is the process for after commit review?



Alex

On 5 December 2016 at 19:25, Vitaly Buka via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

Author: vitalybuka
Date: Mon Dec  5 13:25:00 2016
New Revision: 288689

URL: http://llvm.org/viewvc/llvm-project?rev=288689=rev
Log:
Fix stack-use-after-scope in CheckExplicitlyDefaultedMemberExceptionSpec

Summary:
Similar to r288685.
getExceptionSpec returned structure with pointers to temporarily object
created
by computeImplicitExceptionSpec.

Reviewers: rsmith

Subscribers: aizatsky, cfe-commits

Differential Revision: https://reviews.llvm.org/D27422

Modified:
cfe/trunk/lib/Sema/SemaDeclCXX.cpp

Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=288689=288688=288689=diff
==
--- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Mon Dec  5 13:25:00 2016
@@ -6299,8 +6299,8 @@ void Sema::CheckExplicitlyDefaultedMembe
   CallingConv CC =
Context.getDefaultCallingConvention(/*IsVariadic=*/false,

/*IsCXXMethod=*/true);
   FunctionProtoType::ExtProtoInfo EPI(CC);
-  EPI.ExceptionSpec = computeImplicitExceptionSpec(*this,
MD->getLocation(), MD)
-  .getExceptionSpec();
+  auto IES = computeImplicitExceptionSpec(*this, MD->getLocation(), MD);
+  EPI.ExceptionSpec = IES.getExceptionSpec();
   const FunctionProtoType *ImplicitType = cast(
 Context.getFunctionType(Context.VoidTy, None, EPI));



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r288689 - Fix stack-use-after-scope in CheckExplicitlyDefaultedMemberExceptionSpec

2016-12-06 Thread Alex L via cfe-commits 
Hi Vitaly,

I noticed that you posted this patch up for review at
https://reviews.llvm.org/D27422, but then committed it instantly without
waiting for approval (and you did the same for r288685 as well). Is there
any particular reason why you did this? I think that you should've waited
for approval before committing. This patch is pretty small, and so it looks
to me like it could have been reviewed after it was committed, but patches
that get post-commit reviews shouldn't get explicit review requests like
this one did.

Alex

On 5 December 2016 at 19:25, Vitaly Buka via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: vitalybuka
> Date: Mon Dec  5 13:25:00 2016
> New Revision: 288689
>
> URL: http://llvm.org/viewvc/llvm-project?rev=288689=rev
> Log:
> Fix stack-use-after-scope in CheckExplicitlyDefaultedMemberExceptionSpec
>
> Summary:
> Similar to r288685.
> getExceptionSpec returned structure with pointers to temporarily object
> created
> by computeImplicitExceptionSpec.
>
> Reviewers: rsmith
>
> Subscribers: aizatsky, cfe-commits
>
> Differential Revision: https://reviews.llvm.org/D27422
>
> Modified:
> cfe/trunk/lib/Sema/SemaDeclCXX.cpp
>
> Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/
> SemaDeclCXX.cpp?rev=288689=288688=288689=diff
> 
> ==
> --- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Mon Dec  5 13:25:00 2016
> @@ -6299,8 +6299,8 @@ void Sema::CheckExplicitlyDefaultedMembe
>CallingConv CC = Context.getDefaultCallingConvention(/*
> IsVariadic=*/false,
>
> /*IsCXXMethod=*/true);
>FunctionProtoType::ExtProtoInfo EPI(CC);
> -  EPI.ExceptionSpec = computeImplicitExceptionSpec(*this,
> MD->getLocation(), MD)
> -  .getExceptionSpec();
> +  auto IES = computeImplicitExceptionSpec(*this, MD->getLocation(), MD);
> +  EPI.ExceptionSpec = IES.getExceptionSpec();
>const FunctionProtoType *ImplicitType = cast(
>  Context.getFunctionType(Context.VoidTy, None, EPI));
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r288689 - Fix stack-use-after-scope in CheckExplicitlyDefaultedMemberExceptionSpec

2016-12-05 Thread Vitaly Buka via cfe-commits 
Author: vitalybuka
Date: Mon Dec  5 13:25:00 2016
New Revision: 288689

URL: http://llvm.org/viewvc/llvm-project?rev=288689=rev
Log:
Fix stack-use-after-scope in CheckExplicitlyDefaultedMemberExceptionSpec

Summary:
Similar to r288685.
getExceptionSpec returned structure with pointers to temporarily object created
by computeImplicitExceptionSpec.

Reviewers: rsmith

Subscribers: aizatsky, cfe-commits

Differential Revision: https://reviews.llvm.org/D27422

Modified:
cfe/trunk/lib/Sema/SemaDeclCXX.cpp

Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=288689=288688=288689=diff
==
--- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Mon Dec  5 13:25:00 2016
@@ -6299,8 +6299,8 @@ void Sema::CheckExplicitlyDefaultedMembe
   CallingConv CC = Context.getDefaultCallingConvention(/*IsVariadic=*/false,
/*IsCXXMethod=*/true);
   FunctionProtoType::ExtProtoInfo EPI(CC);
-  EPI.ExceptionSpec = computeImplicitExceptionSpec(*this, MD->getLocation(), 
MD)
-  .getExceptionSpec();
+  auto IES = computeImplicitExceptionSpec(*this, MD->getLocation(), MD);
+  EPI.ExceptionSpec = IES.getExceptionSpec();
   const FunctionProtoType *ImplicitType = cast(
 Context.getFunctionType(Context.VoidTy, None, EPI));
 


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits