Add 'remark' diagnostic type in clang
Richard Smith
richard at metafoo.co.uk
Thu Feb 27 18:57:49 PST 2014
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?
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). I think we should also reject
-Werror=some-remark.
The patch itself looks good. 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?
--- 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.
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140227/ea56bdec/attachment.html>
More information about the cfe-commits
mailing list