This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATH] Patch to fix -finstrument-functions-exclude-function-list handling of namespaces and escaped commas


On 7/4/19 2:18 AM, Oliver Browne wrote:
> See below for modified patch, indentation and newline for curly braces
> style applied, and commented out chunk removed. Apologies, indentation
> and newline for scope are not they way I normally write things, habits
> got the better of me, and I forgot to remove the commented out chunk
> before submission.
> 
> Adding the scope braces and writing the for loop in ordinary C instead
> of relying on a macro are changes made for the sake of
> maintainability.
The right way to do this in GCC is to use the facilities we have
designed for these purposes.  In some cases there are constraints that
those facilities enforce and doing an open-coded implementation will not
work the way you want (the immediate use iterators immediately come to
mind) and in other cases the facilities we've built may be much faster
(the bitmap iterators) and in some cases it's just syntatic sugar, but
the consistency with the rest of the codebase is important.

When you don't follow conventions, the maintainer looking at your patch
has to disentangle the real change from your formatting and convention
changes, then apply the fix by hand, possibly getting it wrong in the
process (made even more possible by the lack of a test in the patch).
THen because they made changes, they'll have to sanity tests to make
sure they didn't goof anything with a typo, etc which takes even more of
their limited time.

If you make it easy to review the patch by following conventions, not
making extraneous changes, include tests, indicate that you regression
tested your patch, etc, then it becomes very easy for a maintainer to
look at the patch and gate it in.

Anyway, I've fixed up your patch to follow existing conventions,
bootstrapped and regression tested it and installed the patch on the trunk.

jeff


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]