[cfe-dev] Warning options table

[Chris Lattner]

On Feb 27, 2009, at 2:50 PM, Sebastian Redl 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.

>> 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. :(

> 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