[PATCH] Store warning option for custom diagnostic messages.

[Alp Toker]

Hi Alexander, I was on the road from the dev meeting in Brussels. Here's 
a summary of my first impressions last week from memory:

The mapping of checker names to warning flags looks a little unnatural. 
Was that the intention and is it a desired artefact that warning flags 
can conflict with built-in warning names? -W flags are well-known and 
get chosen in some cases for compatibility with gcc, so it may be better 
to have a different mechanism to communicate these.

Instead of extending getCustomDiagID() in an ad-hoc fashion, I'd really 
prefer to move towards making TableGen available to describe plugin 
diagnostics sharing the existing syntax we have for built-in diagnostics 
in td files. We should avoid developing two parallel ways of describing 
diagnostics if possible.

One option that's close to your approach and patch may be to associate 
an opaque token with each custom diag ID instead of trying to carry the 
checker name information in -W.

Jordan, what are your thoughts on this?

I'll take a look now at the other patches.

Alp.


On 05/02/2014 19:06, Alexander Kornienko wrote:
> Alp, ping.
>
>
> On Mon, Feb 3, 2014 at 8:36 PM, Alexander Kornienko <alexfh at google.com 
> <mailto:alexfh at google.com>> wrote:
>
>     Alp, ping.
>
>
>
>     On Thu, Jan 30, 2014 at 11:58 PM, Alexander Kornienko
>     <alexfh at google.com <mailto:alexfh at google.com>> wrote:
>
>         On Thu, Jan 30, 2014 at 8:05 PM, Alp Toker <alp at nuanti.com
>         <mailto:alp at nuanti.com>> wrote:
>
>
>             On 30/01/2014 18:29, Jordan Rose wrote:
>
>                  Alp was trying to move away from getCustomDiagID, so
>                 you should probably talk to him about this.
>
>
>             Thanks for pinging Jordan, that's right.
>
>             Alex, can you provide an example of how you intend to use
>             this facility, say a patch making use of it with a test
>             case to get the full picture?
>
>
>         One example of usage is in this patch:
>         http://llvm-reviews.chandlerc.com/D2661 (the test is in
>         clang/tools/extra/test/clang-tidy/basic.cpp). Another usage
>         may appear in the static analyzer, if we go forward with the
>         checker name forwarding. The idea is, that both clang-tidy and
>         the static analyzer have a concept of named checks (or
>         checkers), that can be enabled or disabled from the command
>         line. It is similar to built-in diagnostics in clang, except
>         for the difference in the option syntax (for clang-tidy it's
>         -checks=<regex> and -disable-checks=<regex> instead of
>         -W[no-]diagnostic-name). It seems to be reasonable to use
>         something similar to getWarningOptionForDiag to pass check
>         names through the DiagnosticEngine. Currently this is not
>         available for custom diagnostics.
>
>         Both clang-tidy and the static analyzer use custom
>         diagnostics, as they are built optionally, and we can't
>         statically generate diagnostics and make clang pick them up in
>         the way built-in diagnostics are handled. Furthermore, we have
>         plans for clang-tidy to support custom out-of-tree check
>         modules, which would make this even more complicated. So we
>         have to use custom diagnostics, and I propose adding making
>         some features of built-in diagnostics available for custom
>         diagnostics (for now, getWarningOptionForDiag).
>
>             There's a place for getCustomDiagID() so we wont phase it
>             away entirely but I've got a feeling there might be a
>             cleaner solution in this instance. 
>
>
>         If you have a better idea, I'd be happy to discuss it.
>
>
>             (As for the patch, there are a couple of nits. I'll
>             provide an inline review if we do want to go with it.)
>
>             Alp.
>
>

-- 
http://www.nuanti.com
the browser experts