diff mbox series

[1/2] kernel-doc: don't let V=1 change outcome

Message ID 20230602230014.a435aab03cee.I21ab3b54eeebd638676bead3b2f87417944e44f3@changeid
State New
Headers
Series [1/2] kernel-doc: don't let V=1 change outcome |

Commit Message

Johannes Berg June 2, 2023, 9 p.m. UTC 
  From: Johannes Berg <johannes.berg@intel.com>

The kernel-doc script currently reports a number of issues
only in "verbose" mode, but that's initialized from V=1
(via KBUILD_VERBOSE), so if you use KDOC_WERROR=1 then
adding V=1 might actually break the build. This is rather
unexpected.

Change kernel-doc to not change its behaviour wrt. errors
(or warnings) when verbose mode is enabled, but rather add
separate warning flags (and -Wall) for it.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 scripts/kernel-doc | 41 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

Comments

Masahiro Yamada June 5, 2023, 12:36 a.m. UTC | #1 
On Sat, Jun 3, 2023 at 6:00 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> From: Johannes Berg <johannes.berg@intel.com>
>
> The kernel-doc script currently reports a number of issues
> only in "verbose" mode, but that's initialized from V=1
> (via KBUILD_VERBOSE), so if you use KDOC_WERROR=1 then
> adding V=1 might actually break the build. This is rather
> unexpected.

Agree.


>
> Change kernel-doc to not change its behaviour wrt. errors
> (or warnings) when verbose mode is enabled, but rather add
> separate warning flags (and -Wall) for it.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  scripts/kernel-doc | 41 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
> index 2486689ffc7b..1eb1819fbe13 100755
> --- a/scripts/kernel-doc
> +++ b/scripts/kernel-doc
> @@ -23,7 +23,7 @@ kernel-doc - Print formatted kernel documentation to stdout
>
>  =head1 SYNOPSIS
>
> - kernel-doc [-h] [-v] [-Werror]
> + kernel-doc [-h] [-v] [-Werror] [-Wreturn] [-Wshort-description] [-Wcontents-before-sections] [-Wall]
>     [ -man |
>       -rst [-sphinx-version VERSION] [-enable-lineno] |
>       -none
> @@ -133,6 +133,9 @@ my $dohighlight = "";
>
>  my $verbose = 0;
>  my $Werror = 0;
> +my $Wreturn = 0;
> +my $Wshort_desc = 0;
> +my $Wcontents_before_sections = 0;
>  my $output_mode = "rst";
>  my $output_preformatted = 0;
>  my $no_doc_sections = 0;
> @@ -191,6 +194,24 @@ if (defined($ENV{'KDOC_WERROR'})) {
>         $Werror = "$ENV{'KDOC_WERROR'}";
>  }
>
> +if (defined($ENV{'KDOC_WRETURN'})) {
> +       $Wreturn = "$ENV{'KDOC_WRETURN'}";
> +}
> +
> +if (defined($ENV{'KDOC_WSHORT_DESC'})) {
> +       $Wshort_desc = "$ENV{'KDOC_WSHORT_DESC'}";
> +}
> +
> +if (defined($ENV{'KDOC_WCONTENTS_BEFORE_SECTION'})) {
> +       $Wcontents_before_sections = "$ENV{'KDOC_WCONTENTS_BEFORE_SECTION'}";
> +}
> +
> +if (defined($ENV{'KDOC_WALL'})) {
> +       $Wreturn = "$ENV{'KDOC_WALL'}";
> +       $Wshort_desc = "$ENV{'KDOC_WALL'}";
> +       $Wcontents_before_sections = "$ENV{'KDOC_WALL'}";
> +}



Adding an environment variable to each of them is tedious.


If you enable -Wall via the command line option,
these lines are unneeded?

For example,

ifneq ($(KBUILD_EXTRA_WARN),)
  cmd_checkdoc = $(srctree)/scripts/kernel-doc -none \
         $(if $(findstring 2, $(KBUILD_EXTRA_WARN)), -Wall) $<
endif
Johannes Berg June 5, 2023, 8:18 a.m. UTC | #2 
On Mon, 2023-06-05 at 09:36 +0900, Masahiro Yamada wrote:

> > +if (defined($ENV{'KDOC_WRETURN'})) {
> > +       $Wreturn = "$ENV{'KDOC_WRETURN'}";
> > +}
> > +
> > +if (defined($ENV{'KDOC_WSHORT_DESC'})) {
> > +       $Wshort_desc = "$ENV{'KDOC_WSHORT_DESC'}";
> > +}
> > +
> > +if (defined($ENV{'KDOC_WCONTENTS_BEFORE_SECTION'})) {
> > +       $Wcontents_before_sections = "$ENV{'KDOC_WCONTENTS_BEFORE_SECTION'}";
> > +}
> > +
> > +if (defined($ENV{'KDOC_WALL'})) {
> > +       $Wreturn = "$ENV{'KDOC_WALL'}";
> > +       $Wshort_desc = "$ENV{'KDOC_WALL'}";
> > +       $Wcontents_before_sections = "$ENV{'KDOC_WALL'}";
> > +}
> 
> 
> 
> Adding an environment variable to each of them is tedious.

Agree. And adding one for -Wall is especially tedious because you have
to spell out the list of affected warnings in two places :)

> If you enable -Wall via the command line option,
> these lines are unneeded?
> 
> For example,
> 
> ifneq ($(KBUILD_EXTRA_WARN),)
>   cmd_checkdoc = $(srctree)/scripts/kernel-doc -none \
>          $(if $(findstring 2, $(KBUILD_EXTRA_WARN)), -Wall) $<
> endif
> 

Yes, that should be possible.

I feel like maybe we should still have individual settings for the three
different classes, because you might want to have -Wshort-desc without
the extra -Wcontents-before-sections (which I thought about removing
entirely, kernel-doc seems to parse just fine that way, what's the point
of it?)

But we could even move the env var handling _completely_ to the Makefile
if you don't mind, and then we don't have to have two places in the
script that need to be aligned all the time.

johannes
Masahiro Yamada June 6, 2023, 4:21 a.m. UTC | #3 
On Mon, Jun 5, 2023 at 5:19 PM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Mon, 2023-06-05 at 09:36 +0900, Masahiro Yamada wrote:
>
> > > +if (defined($ENV{'KDOC_WRETURN'})) {
> > > +       $Wreturn = "$ENV{'KDOC_WRETURN'}";
> > > +}
> > > +
> > > +if (defined($ENV{'KDOC_WSHORT_DESC'})) {
> > > +       $Wshort_desc = "$ENV{'KDOC_WSHORT_DESC'}";
> > > +}
> > > +
> > > +if (defined($ENV{'KDOC_WCONTENTS_BEFORE_SECTION'})) {
> > > +       $Wcontents_before_sections = "$ENV{'KDOC_WCONTENTS_BEFORE_SECTION'}";
> > > +}
> > > +
> > > +if (defined($ENV{'KDOC_WALL'})) {
> > > +       $Wreturn = "$ENV{'KDOC_WALL'}";
> > > +       $Wshort_desc = "$ENV{'KDOC_WALL'}";
> > > +       $Wcontents_before_sections = "$ENV{'KDOC_WALL'}";
> > > +}
> >
> >
> >
> > Adding an environment variable to each of them is tedious.
>
> Agree. And adding one for -Wall is especially tedious because you have
> to spell out the list of affected warnings in two places :)
>
> > If you enable -Wall via the command line option,
> > these lines are unneeded?
> >
> > For example,
> >
> > ifneq ($(KBUILD_EXTRA_WARN),)
> >   cmd_checkdoc = $(srctree)/scripts/kernel-doc -none \
> >          $(if $(findstring 2, $(KBUILD_EXTRA_WARN)), -Wall) $<
> > endif
> >
>
> Yes, that should be possible.
>
> I feel like maybe we should still have individual settings for the three
> different classes, because you might want to have -Wshort-desc without
> the extra -Wcontents-before-sections (which I thought about removing
> entirely, kernel-doc seems to parse just fine that way, what's the point
> of it?)
>
> But we could even move the env var handling _completely_ to the Makefile
> if you don't mind, and then we don't have to have two places in the
> script that need to be aligned all the time.

Yes, probably that will be a good idea.
diff mbox series

Patch

diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index 2486689ffc7b..1eb1819fbe13 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -23,7 +23,7 @@  kernel-doc - Print formatted kernel documentation to stdout
 
 =head1 SYNOPSIS
 
- kernel-doc [-h] [-v] [-Werror]
+ kernel-doc [-h] [-v] [-Werror] [-Wreturn] [-Wshort-description] [-Wcontents-before-sections] [-Wall]
    [ -man |
      -rst [-sphinx-version VERSION] [-enable-lineno] |
      -none
@@ -133,6 +133,9 @@  my $dohighlight = "";
 
 my $verbose = 0;
 my $Werror = 0;
+my $Wreturn = 0;
+my $Wshort_desc = 0;
+my $Wcontents_before_sections = 0;
 my $output_mode = "rst";
 my $output_preformatted = 0;
 my $no_doc_sections = 0;
@@ -191,6 +194,24 @@  if (defined($ENV{'KDOC_WERROR'})) {
 	$Werror = "$ENV{'KDOC_WERROR'}";
 }
 
+if (defined($ENV{'KDOC_WRETURN'})) {
+	$Wreturn = "$ENV{'KDOC_WRETURN'}";
+}
+
+if (defined($ENV{'KDOC_WSHORT_DESC'})) {
+	$Wshort_desc = "$ENV{'KDOC_WSHORT_DESC'}";
+}
+
+if (defined($ENV{'KDOC_WCONTENTS_BEFORE_SECTION'})) {
+	$Wcontents_before_sections = "$ENV{'KDOC_WCONTENTS_BEFORE_SECTION'}";
+}
+
+if (defined($ENV{'KDOC_WALL'})) {
+	$Wreturn = "$ENV{'KDOC_WALL'}";
+	$Wshort_desc = "$ENV{'KDOC_WALL'}";
+	$Wcontents_before_sections = "$ENV{'KDOC_WALL'}";
+}
+
 # Generated docbook code is inserted in a template at a point where
 # docbook v3.1 requires a non-zero sequence of RefEntry's; see:
 # https://www.oasis-open.org/docbook/documentation/reference/html/refentry.html
@@ -318,6 +339,16 @@  while ($ARGV[0] =~ m/^--?(.*)/) {
 	$verbose = 1;
     } elsif ($cmd eq "Werror") {
 	$Werror = 1;
+    } elsif ($cmd eq "Wreturn") {
+	$Wreturn = 1;
+    } elsif ($cmd eq "Wshort-desc") {
+	$Wshort_desc = 1;
+    } elsif ($cmd eq "Wcontents-before-sections") {
+	$Wcontents_before_sections = 1;
+    } elsif ($cmd eq "Wall") {
+        $Wreturn = 1;
+        $Wshort_desc = 1;
+	$Wcontents_before_sections = 1;
     } elsif (($cmd eq "h") || ($cmd eq "help")) {
 		pod2usage(-exitval => 0, -verbose => 2);
     } elsif ($cmd eq 'no-doc-sections') {
@@ -1748,9 +1779,9 @@  sub dump_function($$) {
     # This check emits a lot of warnings at the moment, because many
     # functions don't have a 'Return' doc section. So until the number
     # of warnings goes sufficiently down, the check is only performed in
-    # verbose mode.
+    # -Wreturn mode.
     # TODO: always perform the check.
-    if ($verbose && !$noret) {
+    if ($Wreturn && !$noret) {
 	    check_return_section($file, $declaration_name, $return_type);
     }
 
@@ -2054,7 +2085,7 @@  sub process_name($$) {
 	    $state = STATE_NORMAL;
 	}
 
-	if (($declaration_purpose eq "") && $verbose) {
+	if (($declaration_purpose eq "") && $Wshort_desc) {
 	    emit_warning("${file}:$.", "missing initial short description on line:\n$_");
 	}
 
@@ -2103,7 +2134,7 @@  sub process_body($$) {
 	}
 
 	if (($contents ne "") && ($contents ne "\n")) {
-	    if (!$in_doc_sect && $verbose) {
+	    if (!$in_doc_sect && $Wcontents_before_sections) {
 		emit_warning("${file}:$.", "contents before sections\n");
 	    }
 	    dump_section($file, $section, $contents);