Warning options table

Hi,

Revisiting the discussion about warning options we had some time ago.
I've now (finally) looked at tblgen and cooked up how I imagine the
warning option definition might look like. I've attached a sample table
definition.

There are three classes. WarningOption is the base of everything. It has
a name, by which the user refers to it, i.e. if the name is "undef" then
the warning is enabled by -Wundef, disabled by -Wno-undef, and made an
error by -Werror=undef.
I think we should also support -Wundef=warn|ignore|error. That's not
what GCC does, but it seems much more logical to me.
If display is 0, the warning is not shown in the options list. This is
important because there's a LOT of warnings, and showing them all would
be very overwhelming. For example, there's no point in showing
null_in_string, null_in_char and null_in_file as separate warnings.

Warning is a warning (WARN, EXTWARN or EXTENSION) as it appears in the
diagnostic definition file. The diag value is the name of the constant.
The default value is the state the warning is in if it is not otherwise
specified. The default value can only be specified for individual
warnings, not warning groups, because resolving the meaning of different
defaults in different groups would be a mess.

WarningGroup is a group of warning options, i.e. it can contain warnings
and other groups. Setting a warning group is equivalent to setting every
single warning it (recursively) contains. The intention is that later
parameters override the values of earlier parameters, so it is possible
to write, e.g.
clang -Weverything=warn -Whard-to-get-rid-of=ignore
I think the list of members should really be a dag type, but I'm not
quite sure how that works.

Next, I'll dive into llvm::cl and how to write tblgen backends, to see
if I can hook my example file up. Comments are of course very welcome.

Sebastian

warn.td (3.61 KB)

Hi Sebastian,

I think this is an excellent start! On this topic I have a few thoughts I wanted to share as part of the discussion, as I also was planning on working on this very soon. I apologize if I am repeating material here, as I cannot locate the original thread.

Here is an example warning declaration in your tablegen file:

  def FloatEqual : Warning {
    let name = "float-equal";
    let description = "Warn about equality comparisons of floating point values";
  }

you then explicitly register it in a group:

  def Everything : WarningGroup {
    let name = "everything";
    let description = "Enable every single warning";
    let members = [
      UnusedMacros, UndefMacros, NullChar, NestedComment,
      FloatEqual, FormatNonLiteral, IntegerTooLarge, IntegerTooLargeForSigned,
      ImplicitFunctionDeclaration,
      PropertyReadonlyAttrs, StrictSelectorMatch
    ];
  }

We can probably make this a lot more declarative by using the TableGen type system and by using inherited values. In this way warnings declare what groups they belong to instead of groups declaring what warnings belong in them. I think this will lead to a far more succinct representation that is also less error prone.

Here is an example of what I mean. I'm going to start with the syntax that I expect to declare "FloatEqual".

   def FloatEqual : SemaWarning<"float-equal", "Warn about equality comparisons of floating point values">;

Here the definition of FloatEqual takes one line. It inherits from SemaWarning, which could be defined as follows:

   class SemaWarning<string name, string description> : Warning<name,

{}

and Warning:

   class Warning<string name, string description> {
     string Name = name;
     string Description = description;
   }

The TableGen backend can then just iterate over definitions that inherit from Warning, SemaWarning, etc. to generate the appropriate groups. There is no need declare "Everything" explicitly.

I think ultimately we'll probably have one tablegen file with all the warnings, and then have separate backends that blast out the Sema warnings for Sema, the Basic warnings for Basic, etc., so we still get a separation between the warnings but -Wall can still reasoning about all the warnings in clang.

Of course the scheme that I have outline assumes hierarchical groups. For groups that may overlap or simply be a subset of different warnings we can explicitly mention them in the Warning class:

   class Warning<string name, string description> {
    string Name = name;
    string Description = description;
    bit Group1 = 0;
   }

Then if a certain warning wants to be in Group1:

   def SpecialWarning : SemaWarning<"special", "A special warning in Group1"> {
     let Group1 = 1;
   }

The TableGen backend can then just iterate over all warnings and select the ones with Group1 = 1.

My thought on this approach is that groups are completely declarative. There is no risk of not accidentally mentioning a warning in a separate group definition as the declaration of a warning and the declaration of what groups it belongs to are in the same spot. It is also easier to instantly see if a warning is in a specific group. We then can use the tablegen backend to determine what warnings are in a group.

Thoughts?

Ted Kremenek wrote:

Hi Sebastian,

I think this is an excellent start! On this topic I have a few
thoughts I wanted to share as part of the discussion, as I also was
planning on working on this very soon. I apologize if I am repeating
material here, as I cannot locate the original thread.

We can probably make this a lot more declarative by using the TableGen
type system and by using inherited values. In this way warnings
declare what groups they belong to instead of groups declaring what
warnings belong in them. I think this will lead to a far more
succinct representation that is also less error prone.

My thought on this approach is that groups are completely
declarative. There is no risk of not accidentally mentioning a
warning in a separate group definition as the declaration of a warning
and the declaration of what groups it belongs to are in the same
spot. It is also easier to instantly see if a warning is in a
specific group. We then can use the tablegen backend to determine
what warnings are in a group.

Thoughts?

That sounds very good. My only concern with this approach is that it
makes it difficult to have several overlapping hierarchies. That is,
aside from the main hierarchy (which will probably be used for the
Clang-internal component separation, but is of little interest to the
user), all other groups are basically enumerations of single warnings
(although declared the other way around). In other words, I can't make a
whole group part of another group.

Though I suppose I can do that simply with name equality, i.e.

def Warning1 : SemaWarning<"foo", "bar"> {
  let Group1 = 1;
}

def Group1 : WarningGroup<"group1", "blabla"> {
  let Group2 = 1;
}

where groups only need to be explicitly declared if they are actually
members of another group.

Thanks for the comments. I'll see what I can come up with in the way of
a generator over the weekend.

In the meantime, I have a question about the command line. I can think
of two ways of encoding warning options. Both have interesting problems.

1) One cl::opt option per warning, plus a special one for -Werror.
Pro: Automatically generates documentation for every warning. If I only
support GCC syntax (-Wfoo and -Wno-foo) the existing parser for
boolOrUnspec (or whatever it was called) is sufficient. If I only
support property syntax (-Wfoo=warn and -Wfoo=ignore) it is, too. I
probably need a custom parser if I want to support both.
Contra: May be too verbose in the description. Makes position
information hard to come by. In particular, I can't just walk through
the options in the order specified and enable and disable them as I go,
since I'd first have to query them all and sort them. I could probably
build a z-buffer like map of the position of the option that last
modified a warning, and ignore options that have a lower order. Example:
clang -Wall -Wno-cxx-compat -Wcxx-keyword
Suppose I first look at the All group, then at individual warnings, and
finally at custom sub-groups. Then I'd first find the -Wall option and
enable every warning, setting their z value to 1. While walking through
the individual warnings, I find the -Wcxx-keyword option and enable that
warning again, setting its z value to 3. Then I find the -Wno-cxx-compat
group and disable all warnings in that group, setting their z value to
2, except cxx-keyword, which has a higher z value.
That would work, but I feel that walking over every possible warning
option, instead of just those that were actually specified, may impose a
significant startup penalty.

2) One cl::opt for -W, with values being specially interpreted.
Pro: Tells me exactly which options were specified, and in which order.
This means I only need to look at those warnings that actually matter. I
can easily do custom stuff to the values easily.
Contra: One documentation block for all warnings. This means a very,
very long documentation string, and it has to be tblgenerated. I have to
do all the parsing myself, including special treatment of -Werror. It
conflicts with -Wl,foo and friends (are these ever given to clang?).

Ideas? Chris? Can I specify the options separately and still easily walk
them in order?

Sebastian

That sounds very good. My only concern with this approach is that it
makes it difficult to have several overlapping hierarchies. That is,
aside from the main hierarchy (which will probably be used for the
Clang-internal component separation, but is of little interest to the
user), all other groups are basically enumerations of single warnings
(although declared the other way around). In other words, I can't make a
whole group part of another group.

Sure you can. Just do subclassing. The "Group1" fields are only necessary for groups that aren't hierarchical. For all other groups you can just query if a definition inherits from SemaWarning, etc. For example, we could have a another warning group that subclasses SemaWarning. From my understanding of TableGen, the backend has complete access to the tablegen type system and its hierarchy.

Though I suppose I can do that simply with name equality, i.e.

def Warning1 : SemaWarning<"foo", "bar"> {
let Group1 = 1;
}

def Group1 : WarningGroup<"group1", "blabla"> {
let Group2 = 1;
}

I'm not certain what you mean. I don't think you need WarninGroup at all, unless WarningGroup subclasses SemaWarning, etc.

Fred Kremenek wrote:

Hi Sebastian,

I think this is an excellent start! On this topic I have a few
thoughts I wanted to share as part of the discussion, as I also was
planning on working on this very soon. I apologize if I am repeating
material here, as I cannot locate the original thread.

Woo, thanks for working on this Sebastian, this is much needed!

I think the most important part of this is settling on a data model before we convert the majority of the diagnostics over.

Thoughts?

I'd strongly suggest separating out the declaration of the diagnostics from the grouping they are in.

Just describing a diagnostic could be done with a simple line. For example, each diagnostic should be its own "def" as you had it, but can use tblgen classes to simplify them a bit:

def pp_macro_not_used : Warn<"macro is not used">;

The "name of the def" becomes the enum name that the C++ code uses, "Warn" is the classification, and the string is the string :).

It should be really easy to convert over our existing .def files to .td files like this (e.g. with a perl script). Once this is done, we start talking about how to produce warning option info. While some options like "-Wunused-macros" only corresponds to one warning option, other warning options enable/disable many diagnostics. I'd suggest declaring them like this:

def UnusedMacros : WarningOption<
   "unused-macros",
   "Warn for unused macros in the main translation unit",
   ... other flags and stuff ... >;

With a declarative way to define the different warning groups, we now need a way to add the diagnostics to the different controlling sets. This can be done by making 'Warn' and friends take an optional list of warning options they are in. This would give something like:

def pp_macro_not_used : Warn<"macro is not used", [UnusedMacros]>;

For hierarchical warning options, the WarningOption could just have a list of things it includes (e.g. -Wall implies -Wunused-macros).

Another option which is possible but we don't want to do in the first cut is to extend tblgen syntax to allow more flexible names. This would allow something like:

def `unused-macros` : WarningOption< ...

etc, eliminating the need to have the 'UnusedMacros' name in the .td files.

That sounds very good. My only concern with this approach is that it
makes it difficult to have several overlapping hierarchies.

I don't think that overlapping hierarchies should be a problem.

In the meantime, I have a question about the command line. I can think
of two ways of encoding warning options. Both have interesting problems.

2) One cl::opt for -W, with values being specially interpreted.

Right, I'd suggest having a cl::list<string> "W" with cl::prefix. This will give you all the things trailing -W as a vector of strings.

Pro: Tells me exactly which options were specified, and in which order.
This means I only need to look at those warnings that actually matter. I
can easily do custom stuff to the values easily.

Right.

Contra: One documentation block for all warnings. This means a very,
very long documentation string, and it has to be tblgenerated.

The --help output is really of limited utility anyway. :frowning:

I have to
do all the parsing myself, including special treatment of -Werror.

I am not 100% certain, but I think that if you declare an cl::opt that exactly matches -Werror, I think it will get preference to -W as a prefix option.

It
conflicts with -Wl,foo and friends (are these ever given to clang?).

Nope, it should never make it to clang.

Ideas? Chris? Can I specify the options separately and still easily walk
them in order?

cl::list can also tell you the position on the command line that each option was specified.

-Chris

Chris Lattner wrote:

Fred Kremenek wrote:

Hi Sebastian,

I think this is an excellent start! On this topic I have a few
thoughts I wanted to share as part of the discussion, as I also was
planning on working on this very soon. I apologize if I am repeating
material here, as I cannot locate the original thread.

Woo, thanks for working on this Sebastian, this is much needed!

I think the most important part of this is settling on a data model
before we convert the majority of the diagnostics over.

Definitely.

Just describing a diagnostic could be done with a simple line. For
example, each diagnostic should be its own "def" as you had it, but
can use tblgen classes to simplify them a bit:

def pp_macro_not_used : Warn<"macro is not used">;

The "name of the def" becomes the enum name that the C++ code uses,
"Warn" is the classification, and the string is the string :).

Something like this, then?

// Base of all diagnostics
class Diagnostic<string text> {
  string Text = text;
}

// Hard errors
class Error<string text> : Diagnostic<text>;

// Notes, usually contain additional info for a previous diagnostic
class Note<string text> : Diagnostic<text>;

// Warnings, things that are not illegal, but worrisome. Displayed by
default,
// but can be ignored or made into errors with switches.
class Warning<string text> : Diagnostic<text> {
  string Mapping = "warning";
}

// Warnings about extensions, i.e. non-portable code. Ignored by default,
// can be turned into warnings with switches.
class Extension<string text> : Warning<text> {
  let Mapping = "ignore";
}

// Warnings in extension code. Displayed by default. The difference to
normal
// warnings is that these are affected by ErrorOnExtensions.
class ExtensionWarning<string text> : Warning<text>;

It should be really easy to convert over our existing .def files to
.td files like this (e.g. with a perl script).

The preprocessor is enough, I think. Just needs a bit of cleanup
afterwards. The snippet below produces very acceptable code.

#define DIAGKIND_NOTE Note
#define DIAGKIND_WARNING Warning
#define DIAGKIND_EXTENSION Extension
#define DIAGKIND_EXTWARN ExtensionWarning
#define DIAGKIND_ERROR Error

#define DIAG(name, kind, text) \
  def name : DIAGKIND_ ## kind<text>;

#include "DiagnosticCommonKinds.def"

Produces something like:

def note_previous_definition : Note<"previous definition is
here">;

def note_previous_declaration : Note<"previous declaration is here">;

def note_previous_implicit_declaration : Note<"previous implicit
declaration is here">;

def note_previous_use : Note<"previous use is here">;
...

With a declarative way to define the different warning groups, we now
need a way to add the diagnostics to the different controlling sets.
This can be done by making 'Warn' and friends take an optional list of
warning options they are in. This would give something like:

def pp_macro_not_used : Warn<"macro is not used", [UnusedMacros]>;

For hierarchical warning options, the WarningOption could just have a
list of things it includes (e.g. -Wall implies -Wunused-macros).

That's inconsistent. Warnings declare what groups they are in, but
groups declare what other things they include?
Since we separate warnings from warning options completely, I think it
would always be best for the options to declare what they enable. Then
the diagnostic definition doesn't need to care at all about it. In fact,
we could then put the whole options generation into a separate .td file,
which appeals to me.

Another option which is possible but we don't want to do in the first
cut is to extend tblgen syntax to allow more flexible names. This
would allow something like:

def `unused-macros` : WarningOption< ...

etc, eliminating the need to have the 'UnusedMacros' name in the .td
files.

Yes, that would be nice.

Contra: One documentation block for all warnings. This means a very,
very long documentation string, and it has to be tblgenerated.

The --help output is really of limited utility anyway. :frowning:

OK.

I have to
do all the parsing myself, including special treatment of -Werror.

I am not 100% certain, but I think that if you declare an cl::opt that
exactly matches -Werror, I think it will get preference to -W as a
prefix option.

I'll just have to try it.

It
conflicts with -Wl,foo and friends (are these ever given to clang?).

Nope, it should never make it to clang.

Excellent.

Ideas? Chris? Can I specify the options separately and still easily walk
them in order?

cl::list can also tell you the position on the command line that each
option was specified.

Which is interesting if we want options intermixed with source files to
only affect the source files they come before, e.g.

# Compile the first file with -Wall, but the second with no warnings
except -Wimplicit
clang -c -Wall foo.c -Wno-all -Wimplicit bar.c -Wi-am-ignored

If we don't want this, I only care about relative order.

Sebastian

This looks very reasonable to me, although we might want to further subclass into "SemaWarning" (subclassing Warning) and "SemaError" (subclassing Error) so we can partition the warnings for the individual libraries.

Hi Sebastian,

That's an interesting point. Can you give a (pseudocode) example of what you mean?

Ted Kremenek a écrit :

def pp_macro_not_used : Warn<"macro is not used">;

The "name of the def" becomes the enum name that the C++ code uses,
"Warn" is the classification, and the string is the string :).
      

Something like this, then?

// Base of all diagnostics
class Diagnostic<string text> {
string Text = text;
}

// Hard errors
class Error<string text> : Diagnostic<text>;

// Notes, usually contain additional info for a previous diagnostic
class Note<string text> : Diagnostic<text>;

// Warnings, things that are not illegal, but worrisome. Displayed by
default,
// but can be ignored or made into errors with switches.
class Warning<string text> : Diagnostic<text> {
string Mapping = "warning";
}

// Warnings about extensions, i.e. non-portable code. Ignored by default,
// can be turned into warnings with switches.
class Extension<string text> : Warning<text> {
let Mapping = "ignore";
}

// Warnings in extension code. Displayed by default. The difference to
normal
// warnings is that these are affected by ErrorOnExtensions.
class ExtensionWarning<string text> : Warning<text>;
    
This looks very reasonable to me, although we might want to further subclass into "SemaWarning" (subclassing Warning) and "SemaError" (subclassing Error) so we can partition the warnings for the individual libraries.
  
It may be easier to use a multiple let? I don't remenber the syntax exactly but something like that:

let lib= "sema" in
<many declaration>

it would avoid the need to have type of warning*number of library class explosion... (now that I think about it, using multiclass could probably do the trick)

The "name of the def" becomes the enum name that the C++ code uses,
"Warn" is the classification, and the string is the string :).

Something like this, then?

Yes, exactly. I'd suggest short names like ExtWarn though instead of ExtensionWarning.

It should be really easy to convert over our existing .def files to
.td files like this (e.g. with a perl script).

The preprocessor is enough, I think. Just needs a bit of cleanup
afterwards. The snippet below produces very acceptable code.

Cute!

With a declarative way to define the different warning groups, we now
need a way to add the diagnostics to the different controlling sets.
This can be done by making 'Warn' and friends take an optional list of
warning options they are in. This would give something like:

def pp_macro_not_used : Warn<"macro is not used", [UnusedMacros]>;

For hierarchical warning options, the WarningOption could just have a
list of things it includes (e.g. -Wall implies -Wunused-macros).

That's inconsistent. Warnings declare what groups they are in, but
groups declare what other things they include?

Sure, it would be fine to invert it like we do for diagnostics.

Since we separate warnings from warning options completely, I think it
would always be best for the options to declare what they enable. Then
the diagnostic definition doesn't need to care at all about it. In fact,
we could then put the whole options generation into a separate .td file,
which appeals to me.

The general approach we use with .td files it to have a master .td file that uses the tblgen "include" to pull everything into one big file, then have the tblgen backends slice and dice and emit just want they want for a particular invocation.

Ted mentioned the desire to keep per-library diagnostic catalogs. The typical way we'd handle this is to have one big .td file that updates everything, then have makefile logic that runs tblgen once for each library (to emit the .inc file for each library). If any of the .td inputs have changed, tblgen is rerun for each library, but the .inc file is only overwritten (triggering recompilation of .cpp files that include it) if the contents of the .inc file have changed. This is what we do with the LLVM code generator .td files and it works very elegantly.

I have to
do all the parsing myself, including special treatment of -Werror.

I am not 100% certain, but I think that if you declare an cl::opt that
exactly matches -Werror, I think it will get preference to -W as a
prefix option.

I'll just have to try it.

I tried it today for the "-m" prefix option and it worked.

Ideas? Chris? Can I specify the options separately and still easily walk
them in order?

cl::list can also tell you the position on the command line that each
option was specified.

Which is interesting if we want options intermixed with source files to
only affect the source files they come before, e.g.

# Compile the first file with -Wall, but the second with no warnings
except -Wimplicit
clang -c -Wall foo.c -Wno-all -Wimplicit bar.c -Wi-am-ignored

If we don't want this, I only care about relative order.

Right, warning options don't work this way: "the last one wins", so you just care about relative ordering.

-Chris

We don't want that, for two reasons.
1) Combinatorial explosion. It would give us (given the current .def files)
30 base classes to select from. This problem is easily solved, though, by
making Sema etc. separate marker classes. Then you have

def note_previous : Sema, Note<"previously occurred here">;

2) But more importantly, I simply think that separate files, the way we
have them now, are the better choice. So we have a SemaDiag.td, a
LexDiag.td, etc.

Sebastian

This looks very reasonable to me, although we might want to further
subclass into "SemaWarning" (subclassing Warning) and
"SemaError" (subclassing Error) so we can partition the warnings for
the individual libraries.

We don't want that, for two reasons.
1) Combinatorial explosion. It would give us (given the current .def files)
30 base classes to select from. This problem is easily solved, though, by
making Sema etc. separate marker classes. Then you have

def note_previous : Sema, Note<"previously occurred here">;

Right.

2) But more importantly, I simply think that separate files, the way we
have them now, are the better choice. So we have a SemaDiag.td, a
LexDiag.td, etc.

For that, it is probably best to just have:

let Library = "sema" in {

at the top of each per-library file, and } at the end.

-Chris

We don't want that, for two reasons.
1) Combinatorial explosion. It would give us (given the current .def files)
30 base classes to select from. This problem is easily solved, though, by
making Sema etc. separate marker classes. Then you have

def note_previous : Sema, Note<"previously occurred here">;

Right.

Makes sense.

2) But more importantly, I simply think that separate files, the way we
have them now, are the better choice. So we have a SemaDiag.td, a
LexDiag.td, etc.

For that, it is probably best to just have:

let Library = "sema" in {

at the top of each per-library file, and } at the end.

-Chris

Does having separate per-library files pose issues with implementing -Wall? That is, to implement -Wall, or any other option whose warnings might cross several libraries, don't we need the ability to potentially know about all the warnings across Clang? I think that was the original motivation to put all the warnings in one TableGen file.

We want each library to put its diags in separate .td files with a "let" wrapping them, then we want one file that pulls in everything:

Diagnostics.td:

include "SemaDiags.td"
include "ParserDiags.td"
...

The tblgen backend should be invoked once for each library with the full set of input, and only emit the diags corresponding to each invocation. e.g,

tblgen Diagnostics.td -emit-clang-diags-info -clang-diags-library=Sema -o Sema.inc
tblgen Diagnostics.td -emit-clang-diags-info -clang-diags-library=Parser -o Parser.inc
...

-Chris

Right! Sounds good to me.

If we have the warning option definitions in a separate file anyway (and I
think we should, since warning options are orthogonal to which component
issues the warning), you can just have that file include all the diagnostic
definition files.

Sebastian

Hi Sebastian,

I've been looking at what you have done and have started work on the TableGen backends.

Concerning your point above, I still think that warning groups need to be declarative, where a warning says (either implicitly or explicitly) that it is part of a warning group. Explicitly having to declare warning groups seems highly suboptimal.

We can then have a separate .td entry that tells how a warning group maps to a command-line option.

Ted

Ted Kremenek wrote:

Concerning your point above, I still think that warning groups need to
be declarative, where a warning says (either implicitly or explicitly)
that it is part of a warning group. Explicitly having to declare
warning groups seems highly suboptimal.

We can then have a separate .td entry that tells how a warning group
maps to a command-line option.

What is the point of warning groups, other than being able to control
several warnings with a single option? The organizational units (Sema,
Parse, ...) aside, I mean.

In the files I checked in, there aren't any warning groups any more
(again, not counting Sema & friends). There's warnings, and there's
warning options. Each warning option specifies which warnings it
controls, which I feel is exactly the way it should be. Purely for
convenience, a warning option can include all warnings of another
warning option by specifying that.

Sebastian

Ted Kremenek wrote:

Concerning your point above, I still think that warning groups need to
be declarative, where a warning says (either implicitly or explicitly)
that it is part of a warning group. Explicitly having to declare
warning groups seems highly suboptimal.

We can then have a separate .td entry that tells how a warning group
maps to a command-line option.

What is the point of warning groups, other than being able to control
several warnings with a single option?

Hi Sebastian,

I'm a little confused. That is the whole point. The idea is to have a declarative way to specify what warnings belong to what groups. A warning option then maps to a specific warning or a warning group. The idea is to declaratively specify warning groups, and then have a warning option either map to a single warning (which can be thought of as a singleton warning group) or a warning group.

The approach you describe conflates warning groups and warning options. Warning options are part of the command line interface, where warning groups are part of the underlying "API interface" to warnings. Another client of the Clang libraries may wish to turn off groups of warnings without reasoning about warning options.

I see these merely as a separation of concerns. Warning options are a command-line interface concept, where warning groups are a Clang library concept.

The organizational units (Sema,
Parse, ...) aside, I mean.

In the files I checked in, there aren't any warning groups any more
(again, not counting Sema & friends).

Yes, this discussion is not about Sema & friends.

There's warnings, and there's
warning options. Each warning option specifies which warnings it
controls, which I feel is exactly the way it should be. Purely for
convenience, a warning option can include all warnings of another
warning option by specifying that.

I think some of my confusion would be resolved if you gave some example code (e.g., TableGen pseudocode) of how you plan on mapping warning options to warnings. We may actually be agreeing with each other, and some code might clarify some things.

Ted Kremenek wrote:

Ted Kremenek wrote:

Concerning your point above, I still think that warning groups need to
be declarative, where a warning says (either implicitly or explicitly)
that it is part of a warning group. Explicitly having to declare
warning groups seems highly suboptimal.

We can then have a separate .td entry that tells how a warning group
maps to a command-line option.

What is the point of warning groups, other than being able to control
several warnings with a single option?

Hi Sebastian,

I'm a little confused.

Sorry, should have used the term "single command line option".

That is the whole point. The idea is to have a declarative way to
specify what warnings belong to what groups. A warning option then
maps to a specific warning or a warning group. The idea is to
declaratively specify warning groups, and then have a warning option
either map to a single warning (which can be thought of as a singleton
warning group) or a warning group.

OK. So we basically separate the grouping of warnings from deciding on a
user-visible string for that group?

The approach you describe conflates warning groups and warning
options. Warning options are part of the command line interface,
where warning groups are part of the underlying "API interface" to
warnings.

I never intended them to be visible in the API interface, but I suppose
that makes sense.

Another client of the Clang libraries may wish to turn off groups of
warnings without reasoning about warning options.

True.

There's warnings, and there's
warning options. Each warning option specifies which warnings it
controls, which I feel is exactly the way it should be. Purely for
convenience, a warning option can include all warnings of another
warning option by specifying that.

I think some of my confusion would be resolved if you gave some
example code (e.g., TableGen pseudocode) of how you plan on mapping
warning options to warnings. We may actually be agreeing with each
other, and some code might clarify some things.

I expected warning options to simply list the warnings they control:

def OptExtra : Option<"extra", [GCCAll, EffectiveCXX,
warn_sign_compare_mismatch, warn_pointer_zero_relation]>;

This would expand to something like:

static const diag::kind OptExtraDiags[] = {
  diag::warn_sign_compare_mismatch, diag::warn_pointer_zero_relation, /*
all of GCCAll */, /* all of EffectiveCXX */
};
// ...
static const WarningOption OptionTable[] = {
  /* ... */
  { "extra", DIAGS(OptExtraDiags) },
  /* ... */
};

But if we expose warning options through some other means than the
command line (and pragma warning, which I want to use identical names),
then I think moving the association declaration to the warning makes sense.

Sebastian