Re: [v4l-dvb-maintainer] [PATCH] media: replace remaining __FUNCT ION__ occurences

Harvey Harrison wrote:
On Wed, 2008-04-09 at 13:00 -0400, Michael Krufky wrote:

On Fri, Apr 4, 2008 at 11:09 AM, Michael Krufky <mkrufky@xxxxxxxxxxx>
Harvey,

I have received your entire patchset. Some patches have already been
merged into our development tree, others have been dropped, since some
of individual driver maintainers have decided to remove the
__FUNCTION__ macro from their source code altogether, rather than
accept this change.

I have merged the remaining pending patches into a mercurial tree,
hosted on linuxtv.org:

http://linuxtv.org/hg/~mkrufky/function-func

Please note that I had to manually apply patches 8, 11 and 13, since
you generated your changes against the git repository rather than the
official v4l-dvb development repository hosted on linuxtv.org.


I don't know/use mercurial, sorry, I thought git-v4l's devel branch on
kernel.org would be a mirror of the development tree...guess I was
mistaken


The v4l-dvb git tree is where changesets go before Mauro requests a
merge to Linus. This repository is usually months behind the current
development tree, and for good reason.

Mauro's 'devel' branch is relatively close to the mercurial repository,
however, in the mercurial repository we maintain backwards compatibility
that usually goes back at least a few kernel versions. In order to
support that compatibility, there are extra #ifdef compiler directives
in that source tree, which is stripped away before merging to -git.

When large patches come in based on the git repositories, a large amount
of manual application is needed in order to merge properly. This is
usually only an issue for large, frivolous coding style changes. Most
functional patches never hit this issue.

I must stress this -- all v4l-dvb patches, ESPECIALLY
codingstyle-cleanups (due to the nature of those patches, touching
many many files at once), should always be generated against the
v4l-dvb master development repository hosted on linuxtv.org.

Now, I have a question.....

About this change from __FUNCTION__ to __func__ , I understand that
this change is being done kernel-wide. At first, I had blindly
accepted this change as a kernel-janitor "cleanup", until it was
pointed out to me last night, that older compilers do not support
__func__. Sure, one can always do the following for compat:

#ifndef __func__
#define __func__ __FUNCTION__
#endif /* __func__ */


This is already done in kernel.h, so __func__ is already being passed to
any compiler used on the kernel....

/* Trap pasters of __FUNCTION__ at compile-time */
#define __FUNCTION__ (__func__)


You misunderstood me. I was talking about how people can compile the
sources using __func__ rather than __FUNCTION__ using older compilers
that do not support __func__.

Meanwhile -- you just pointed out this trap, above. If that is already
in kernel.h, then why replace all actual occurences of __FUNCTION__ with
__func__ across the kernel tree? That trap allows your cleanup to take
affect in the compiled binaries, while NOT forcing this change on the
subsystem kernel code.

The v4l-dvb tree is used outside of the kernel as well as within the
kernel, and we like to support additional configurations that are not
necessarily supported in-kernel.

This conversion "cleanup" is starting to sound less and less
attractive. Was there ever a discussion & agreement about this on LKML??


...but the question is raised, why are we making this change in the first
place?

Don't get me wrong -- as I said before, I understand that this change
is kernel-wide, and I am not arguing against it. I will continue on
to have this merged into 2.6.26. I would just like to see a link that
points to a discussion thread on LKML that explains the reasons for
this change, and where this change was globally agreed to. Again -- I
am not challenging these patches. I merely want to read more
information as to why we are making this move.


I'm guessing that this discussion never actually took place?

In the meanwhile, below is the checkpatch.pl fallout after applying
your __FUNCTION__ to __func__ series. Since you are working on these
codingstyle cleanups anyway, I'd imagine that you won't mind fixing
these checkpatch.pl "errors" and "warnings" before we merge these
changes.


For such a large set in v4l, it's a drastic increase in work to do so
in this case as it is a simple sed s/__FUNCTION__/__func__/


Please see my comments at the end of this email.


I understand if you don't want to alter code that you may not be
directly involved in, but I am sure you will have no trouble at least
fixing the "comma after space" and "line over 80 characters" cases.

Please generate the additional cleanups against the mercurial tree
that I merged your previous series to:

http://linuxtv.org/hg/~mkrufky/function-func


Do you have a git mirror somewhere?


No. A git mirror would not help in this case, since we would need to
merge the changes back into the mercurial repository before the changes
waiting there go to -git.


Also, please generate the codingstyle cleanup patches individually
based on the directory structure, just as you did in your last series.

See below for the checkpatch.pl "errors" and "warnings".


I can't say I have much enthusiasm for that, but if you'd really want
such a patch, I will try to get to it this week.
It's not that I "really want such a patch" -- rather, I am merely
reacting in the same manner that has been done in the past.

For example:

Quite often the case arises where a user reports an OOPS, or some other
bug in the current code. A developer will fix the bug, and send in a
patch to fix it. The maintainer of our subsystem then proceeds to NACK
the bug-fix patch, because it fails checkpatch.pl.

I disagree with this policy, however, this is the policy. I always felt
that patches should be small, and only attack one specific item. I have
always agreed with akpm's "The Perfect Patch" document, where no two
changes should ever appear in the same patch. ie: if there is a
codingstyle / whitespace cleanup, it should appear in a separate patch,
rather than a patch that fixes a bug or adds new functionality.

Ever since the appearance of checkpatch.pl, now all patches that fix
real bugs get nacked unless they themselves pass checkpatch.pl's
"requirements". The patch must be re-worked, or an additional patch
must be submitted, that removes any codingstyle issue detected by the
original patch, even if it is caused by a piece of code that had already
existed.

In summary, I as a developer, am made aware of bugs in the kernel, and I
like to fix them when I can. When I submit a bug-fix patch that alters
a line that has comma's without spaces afterwards, my patch is nacked,
until I fix the codingstyle issue as well, even though the codingstyle
issue pre-dates my own patch.

If I can't submit a bug-fix without being held up to checkpatch.pl ,
then those same requirements must be upheld by trivial, frivolous coding
style "cleanups" as well.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Harvey, I know you mean well, and I thank you for that. I hope you
understand my point of view.

Regards,

Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • 2.6.11-ck2
    ... These are patches designed to improve system responsiveness. ... Split patches and a server specific patch available. ... Staircase scheduler update. ... This brings it in line with his current stable tree. ...
    (Linux-Kernel)
  • Re: Please pull ACPI updates
    ... conflict resolution back into the original patch, ... The reason I maintain patches in quilt is that it's quite easy to ... their tree when my tree wasn't at exactly the same point. ... the originals. ...
    (Linux-Kernel)
  • Re: [PATCH v3 0/3] Add support for compiling with -ffunction-sections -fdata-section
    ... The section rename patch often merge conflicts with other changes. ... tree and Linus' tree. ... agreement of each developer for updating their patches)? ... post the rest of the Ksplice patchset for one of those trees. ...
    (Linux-Kernel)
  • Re: [PATCH] staging: slicoss-use request_firmware
    ... It seems that some of my previous patches ... If this patch is accepted, I can send a patch to remove the static firmware from the tree. ... u32 codeaddr; ...
    (Linux-Kernel)
  • 9_Recommended error codes (specifically return code 5)
    ... * "return code 2" indicates patches are already installed. ... * "return code 25" means a patches requires another patch that is not yet installed. ... With or without using the save option, the patch installation process ... Installing 114008-01... ...
    (SunManagers)