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

[Alex L via cfe-commits]

On 6 December 2016 at 19:39, Vitaly Buka <vitalybuka at google.com> wrote:

> Hi Alex,
>
>
>
> On Tue, Dec 6, 2016 at 11:14 AM Alex L <arphaman at gmail.com> 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 at 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&view=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&r1=288688&r2=288689&view=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<FunctionProtoType>(
>      Context.getFunctionType(Context.VoidTy, None, EPI));
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161207/96316343/attachment-0001.html>