Add 'remark' diagnostic type in clang

[Tobias Grosser]

On 02/28/2014 03:57 AM, Richard Smith wrote:
> On Thu, Feb 27, 2014 at 6:40 PM, Arthur O'Dwyer
> <arthur.j.odwyer at gmail.com>wrote:
>
>> On Thu, Feb 27, 2014 at 6:23 PM, Tobias Grosser <tobias at grosser.es> wrote:
>>
>>> On 02/26/2014 10:27 PM, Tobias Grosser wrote:
>>>
>>>> On 02/26/2014 10:19 PM, Chris Lattner wrote:
>>>>
>>>>> On Feb 26, 2014, at 2:22 AM, Tobias Grosser <tobias at grosser.es> wrote:
>>>>>
>>>>>>
>>>>>>   3) How to enable 'remarks'
>>>>>>
>>>>>> We need a way to enable 'remark' diagnostics. Quentin proposed to go
>>>>>> for an approach similar to the warning flags. Where we control remarks
>>>>>> with '-Rvector', '-Rloop-vector', ...
>>>>>>
>>>>>> I will read a little bit through the existing option system to better
>>>>>> understand what it is doing, possibly adding documentation / cleanups
>>>>>> on my way. I will come back with a proposal here.
>>>>>>
>>>>>
>>>>> It's a bit odd, but since these are diagnostics, why not use the
>>>>> existing -W flags?  You should be able to -Werror one of these,
>>>>> control them with #pragma clang diagnostics, etc.  It doesn't seem
>>>>> like we need more complexity in this space.
>>>>>
>>>>
>>>> Good point. I will prepare the above patches such that they reuse the
>>>> existing infrastructure. If we really see a need for further
>>>> adjustments, we can do this incrementally.
>>>>
>>>
>>> I just updated the patch to reflect the conclusions of our discussion.
>>> Please review for commit.
>>>
>>> ------------
>>> [PATCH] Add 'remark' diagnostic type in 'clang'
>>>
>>>
>>> A 'remark' is information that is not an error or a warning, but rather
>>> some additional information provided to the user. In contrast to a 'note' a
>>> 'remark' is an independent diagnostic, whereas a 'note' always depends on
>>> another diagnostic.
>>>
>>> A typical use case for remark nodes is information provided to the user,
>>> e.g. information provided by the vectorizer about loops that have been
>>> vectorized.
>>>
>>> This patch provides the initial implementation of 'remarks'. It includes
>>> the actual definiton of the remark nodes, their printing as well as basic
>>> parameter handling. We are reusing the existing diagnostic parameters which
>>> means a remark
>>> can be enabled with normal '-Wdiagnostic-name' flags and can be upgraded
>>> to an error using '-Werror=diagnostic-name'.
>>>
>>
>> For the record, I strongly recommend that the syntax to enable a remark
>> should be "-Wremark=diagnostic-name", and that "-Wdiagnostic-name" should
>> (continue to) mean "upgrade this diagnostic to a warning".
>> However, I have no objection to the current semantics for an initial
>> checkin, as long as nobody uses it as an excuse to keep those semantics
>> forever.
>>
>
> I'm not following something here. If it makes sense for a diagnostic to be
> upgraded to an error, why would it be a remark rather than a warning? Put
> another way, if a remark describes something that might indicate a problem,
> then what is the difference between a remark and a warning, and why do we
> need a new kind of thing in the first place?

In my opinion, remarks are pieces of additional information that do not 
necessarily indicate a problem. E.g. the positive notification that a 
loop was indeed vectorized or - in Polly - that an optimizable loop 
region was found and what transformation has been applied.

> I would have thought that the difference is that a remark indicates
> something that *isn't* a problem, and is just informational. That being the
> case, it doesn't make sense to me to upgrade remarks to warnings or to
> errors. It would seem bizarre to me if -Werror converts remarks to errors
> (and indeed, in this patch, it does not).

Good point. I explicitly mentioned that '-Werror' does not trigger on 
remarks in the commit message.

 > I think we should also reject
> -Werror=some-remark.

Several people mentioned that they like this feature including Chris. 
This either needs more discussion or just more experience when some
remarks have been added.

> The patch itself looks good.

Committed in r20247 with the last quibbles being addressed as below.

Some quibbles:
>
> --- a/include/clang/Basic/DiagnosticIDs.h
> +++ b/include/clang/Basic/DiagnosticIDs.h
>       enum Mapping {
>         // NOTE: 0 means "uncomputed".
>         MAP_IGNORE  = 1,     ///< Map this diagnostic to nothing, ignore it.
>         MAP_WARNING = 2,     ///< Map this diagnostic to a warning.
>         MAP_ERROR   = 3,     ///< Map this diagnostic to an error.
> -      MAP_FATAL   = 4      ///< Map this diagnostic to a fatal error.
> +      MAP_REMARK  = 4,     ///< Map this diagnostic to a remark.
> +      MAP_FATAL   = 5      ///< Map this diagnostic to a fatal error.
>       };
>
> Why is REMARK here, rather than between IGNORE and WARNING?

Fixed.

> --- a/lib/Frontend/TextDiagnostic.cpp
> +++ b/lib/Frontend/TextDiagnostic.cpp
> @@ -711,6 +713,7 @@ TextDiagnostic::printDiagnosticLevel(raw_ostream &OS,
>       case DiagnosticsEngine::Ignored:
>         llvm_unreachable("Invalid diagnostic type");
>       case DiagnosticsEngine::Note:    OS.changeColor(noteColor, true);
> break;
> +    case DiagnosticsEngine::Remark: OS.changeColor(remarkColor, true);
> break;
>       case DiagnosticsEngine::Warning: OS.changeColor(warningColor, true);
> break;
>       case DiagnosticsEngine::Error:   OS.changeColor(errorColor, true);
> break;
>       case DiagnosticsEngine::Fatal:   OS.changeColor(fatalColor, true);
> break;
> @@ -721,6 +724,7 @@ TextDiagnostic::printDiagnosticLevel(raw_ostream &OS,
>     case DiagnosticsEngine::Ignored:
>       llvm_unreachable("Invalid diagnostic type");
>     case DiagnosticsEngine::Note:    OS << "note"; break;
> +  case DiagnosticsEngine::Remark:   OS << "remark"; break;
>     case DiagnosticsEngine::Warning: OS << "warning"; break;
>     case DiagnosticsEngine::Error:   OS << "error"; break;
>     case DiagnosticsEngine::Fatal:   OS << "fatal error"; break;
>
> In both cases, whitespace doesn't match the surrounding statements.

Fixed.

> Please also teach TableGen to error if a remark is not within a DiagGroup,
> so we don't end up with the same mess of missing diagnostic options that we
> have for warnings.

Done.