This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] Verify warning flags for CC_FOR_BUILD compiler
- From: Vlad Zakharov <Vladislav dot Zakharov at synopsys dot com>
- To: "binutils at sourceware dot org" <binutils at sourceware dot org>, "nickc at redhat dot com" <nickc at redhat dot com>
- Cc: "Anton dot Kolesov at synopsys dot com" <Anton dot Kolesov at synopsys dot com>, "Alexey dot Brodkin at synopsys dot com" <Alexey dot Brodkin at synopsys dot com>, "Cupertino dot Miranda at synopsys dot com" <Cupertino dot Miranda at synopsys dot com>
- Date: Mon, 26 Sep 2016 10:43:55 +0000
- Subject: Re: [PATCH] Verify warning flags for CC_FOR_BUILD compiler
- Authentication-results: sourceware.org; auth=none
- References: <1473261502-17860-1-git-send-email-vzakhar@synopsys.com> <1474620893.7924.9.camel@synopsys.com> <3614324b-72ee-349e-e792-4f12758d59ea@redhat.com>
Hi Nick,
On Mon, 2016-09-26 at 10:32 +0100, Nick Clifton wrote:
> Hi Vlad,
>
> >
> > Please treat this message as a polite reminder to review the patch.
>
> Oops- sorry for dropping the ball on this one.
>
> I have a couple of questions about the patch:
>
> >
> > >
> > > # Add -Wshadow if the compiler is a sufficiently recent version of GCC.
> > > -AC_EGREP_CPP([^[0-3]$],[__GNUC__],,GCC_WARN_CFLAGS="$GCC_WARN_CFLAGS -Wshadow")
> > > +AC_EGREP_CPP([^[0-3]$],[__GNUC__],,GCC_asdfasdfWARN_CFLAGS="$GCC_WARN_CFLAGS -Wshadow")
>
> What is this for ? Why is the change being made and what is the significance of 'asdfasdf' ?
>
> Other than that though the patch looks good to me. I am withholding approval
> pending an answer to the above question, (which I suspect might turn out to be
> a typo), but I think that we are almost there.
Oh, sorry for that, it is really a typo.
Of course there shouldn't be any updates at that line. It's just my silly
mistake.
>
> One other small point - it is easier for us if you include the proposed ChangeLog
> entries for your patch as plain text rather then context diffs. This is because
> the diff almost never applies directly to the sources, since the changelogs change
> so frequently.
>
> Oh, and it is good practice to omit diffs for auto-generated files like Makefile.in,
> as you have done, but you should mention in the changelog entry that the file has
> been regenerated. Ie:
>
> 2016-09-07 Vlad Zakharov <vzakhar@synopsys.com>
>
> * Makefile.am: Replace AM_CLFAGS with AM_CFLAGS_FOR_BUILD
> when building with CC_FOR_BUILD compiler.
> * Makefile.in: Regenerate.
>
> Cheers
> Nick
>
>
I have taken note of your comments about ChangeLogs and auto-generated files.
Should I resend my patch with these minor
updates?
Thanks.
--
Best regards,
Vlad Zakharov <vzakhar@synopsys.com>