Re: [warning control] New #pragma GCC diagnostic

[DJ Delorie <dj at redhat dot com>]

> If a specific warning has not been requested to be transmuted into
> error, then it should stay as DK_WARNING.  No?  

If a specific DIAGNOSTIC has not been requested to be transmuted into
something else, it should stay as WHATEVER it was before.  You asked
me to move the override into diagnostic_report_diagnostic, so it
affects all kinds of diagnostics controlled by those options, not just
warnings.  If some -Wfoo controls, say, a DK_INFO, this overrides that
too.  For example, some -f option might provide notes, debugs,
whatever.

It only happens to control warnings at the moment because warning() is
the only function that takes an OPT_*.  In theory, we could add an
OPT_* to error() and let the use change it to DK_FATAL through a
#pragma, much like what -Wfatal-errors does.

Anyway, that's why I used DK_UNSPECIFIED instead of DK_WARNING for
that purpose.

> Aha; thanks!  Can you put a comment to that effect in the source?
> I'm likely to forget in few days.

Ok.

> Sorry, I don't understand this part.  I thought the plan is to attach
> the context to the function body, not every single tree/lines.

The current patches only provide for one global state, which is the
array.  Hence the documentation says "only do this before any
functions".  It's been noted that a useful addition to this is to keep
track of the lines where each #pragma occur, and compare those to the
reported line number for the diagnostic, to provide line-level
granularity of control.  We've discussed scope-level control and it's
much more complicated, because you end up with a context attached to
each tree node.

> and we should be able (currently, we are not, but we should be!) to
> call front-ends with different sets of flags.  Libcpp successfully
> behaves that way.  There is no reason why we should duplicate

libcpp is a problem because we don't have a way of enumerating its
diagnostics.  Plus, if we put the overrides in the context, everyone
who uses the context needs to include <options.h>.

To support libcpp with this system, we'd need to add to the api:

* A way of allocating OPT_* numbers
* A way of mapping options to OPT_* numbers
* A way of passing OPT_* number back to gcc.

For example, if we tell libcpp "your options start with 415" (assuming
N_OPTS is 415 for gcc itself), it would need to add 415 to each OPT_*
it passes back to gcc.

> Well, the general idea with -Werror is to turn all warnings into
> errors.  The -Werror=xxx machinery refines that by selectively
> dispensing (or requiring) specific warnings from that behaviour.
> So, with the refinement you're proposing, what we have is this:
> 
>   (1) by defaults, classify_diagnostic[] is full of DK_WARNING

Well, DK_UNSPECIFIED.  Same effect at the moment, because warning()
happens to be the only one that uses this mechanism.

>   (2) when a diagnostic is enabled AND turned into error, the
>       corresponding the correspondong slot is set to DK_ERROR.

Yes.  Or DK_IGNORED.  Or, potentially, DK_INFO.  Or any other kind.
The others just didn't make much sense, and I didn't want to
complicate things (-Winfo=format anyone?), but they'll all work.

> | To get the warnings-as-errors message, we'd have to keep track of
> | whether it's been issued for *each* type of diagnostic, which means
> | another 2Kb of data.
> 
> No, we should not need another 2Kb of data.  We have variables
> (diagnostic_context::diagnostic_count) that keep track of the total
> count of diagnostics.  We have only DK_LAST_DIAGNOSTIC_KIND (8
> currently) ints to maintain.

I was thinking:

warning: comparison always true due to type limits [-Walways-true]
warning: The above warning is being treated as an error [-Werror=always-true]
warning: signedness ignored in function call [-Wsigned]
warning: missing prototype for "foo" [-Wprototypes]
warning: The above warning is being treated as an error [-Werror=prototypes]

Note that the signedness warning is still a warning; we'd need to note
*which* warnings are being treated as errors, hence N_OPTS flags.

My way, we just print "error:" and it becomes obvious, no flags
needed.

error: comparison always true due to type limits [-Walways-true]
warning: signedness ignored in function call [-Wsigned]
error: missing prototype for "foo" [-Wprototypes]

Hmmm... /me wonders if -Werror=foo should imply -Wfoo ?
The pragma logic does.

> |  You'd also have to tailor each of those warnings
> | for the specific command line option it's overriding.  And you
> | wouldn't be able to get messages for ignored warnings anyway.
> | "Warning: I didn't show you a warning about -Wswitch."
> 
> I suppose I don't understand this part.  My mental model of the
> algorithm is this:
> 
>   * a diagnostic that is not enabled is never issued.
>   * a diagnostic that is enabled gets counted in
> 
>       context->diagnostic_count[context->classify_diagnostic[W_option]]
> 
> I don't see why we would have to issue
> 
>    "Warning: I didn't show you a warning about -Wswitch."
> 
> (which could be interesting for fortune :-)

Well, we currently issue a warning when *all* warnings are being
treated as errors.  If we also issued a warning when *some* warnings
were treated as errors, for symmetry we should issue a warning when
some warnings are being treated as *anything* other than a warning.
Consider: "Warning: this warning is being treated like a debug
message.".  Now, how do we tell the user that the warning they should
have gotten with -Wformat, but didn't, isn't being shown?  We can't,
obviously.

My point is that if you can't reasonably use the existing messages for
all the new cases, you need to come up with a new solution that *does*
work in all the new cases.  Replacing "warning:" with "error:" (or
whatever) works in all cases, because you end up replacing a
text-not-displayed with a prefix-not-displayed (i.e. you still do
nothing, but technically are following the same pattern ;).

> | Or *re*classify_diagnostic ;-)
> 
> I suppose it is a matter of perspective.  I see the -Werror=xxx and
> refining pragmas as way of making general statements, followed by
> specific exceptions :-)

To me, the fact that you call warning() or error() or whatever,
classifies the diagnostic its first time.  Everything else after that
is a reclassification.  But it's just semantics, not really important.

> | > There is also the question of the meaning of
> | > issue_warnings_are_errors_message that changes with your patch.
> | 
> | Yeah.  Or worse, when the user says -Werror and the #pragma changes it
> | back to a warning, do you get two messages?  "Warning: warnings
> | treated as errors" "Um, except not this one".
> 
> I suppose not.  That issue disappears when
> issue_warnings_are_errors_message just goes away :-)

We've trained the user that anything prefixed by "warning:" is a
warning, unless we tell them otherwise.  With my patch, if we change
something to no longer be a warning, we stop telling the user it's a
warning.  If we change something to be a not-warning and continue
printing the word "warning:" on it, we're only going to confuse the
user.  Hence, we should stop doing that.  If it's being treated as an
error, print the word "error"!  We should stop with the "this is a
warning" "no, it is an error" messages.

I suggest a follow-up patch that reimplements -Werror as an override
(if DK_WARNING, use DK_ERROR) so that it prints "error:" and counts it
as an error, rather than printing "warning" and having the special
logic in the counting routine.