On Fri, Apr 30, 2021 at 12:36 PM Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote:
> >
> > other comments
> >
> > if (strcmp(defel->defname, "volatility") == 0)
> > {
> > if (is_procedure)
> > - goto procedure_error;
> > + is_procedure_error = true;
> > if (*volatility_item)
> > - goto duplicate_error;
> > + is_duplicate_error = true;
> >
> > Another side effect I see is, in the above check earlier if
> > is_procedure was true it was directly goto procedure_error, but now it
> > will also check the if (*volatility_item) and it may set
> > is_duplicate_error also true, which seems wrong to me. I think you
> > can change it to
> >
> > if (is_procedure)
> > is_procedure_error = true;
> > else if (*volatility_item)
> > is_duplicate_error = true;
>
> Thanks. Done that way, see the attached v3. Let's see what others has to say.
>
> Also attaching Vignesh's v3 patch as-is, just for completion.
Looking into this again, why not as shown below? IMHO, this way the
code will be logically the same as it was before the patch, basically
why to process an extra statement ( *volatility_item = defel;) if we
have already decided to error.
if (is_procedure)
is_procedure_error = true;
else if (*volatility_item)
is_duplicate_error = true;
else
*volatility_item = defel;
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
