On 02/28/2014 03:57 AM, Richard Smith wrote:
On Thu, Feb 27, 2014 at 6:40 PM, Arthur O'Dwyer
<[email protected]>wrote:
On Thu, Feb 27, 2014 at 6:23 PM, Tobias Grosser <[email protected]> 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 <[email protected]> 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.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
