<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Feb 27, 2014 at 6:40 PM, Arthur O'Dwyer <span dir="ltr"><<a href="mailto:arthur.j.odwyer@gmail.com" target="_blank">arthur.j.odwyer@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="">On Thu, Feb 27, 2014 at 6:23 PM, Tobias Grosser <span dir="ltr"><<a href="mailto:tobias@grosser.es" target="_blank">tobias@grosser.es</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div><div class="">On 02/26/2014 10:27 PM, Tobias Grosser wrote:<br>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">
On 02/26/2014 10:19 PM, Chris Lattner wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
On Feb 26, 2014, at 2:22 AM, Tobias Grosser <<a href="mailto:tobias@grosser.es" target="_blank">tobias@grosser.es</a>> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br></blockquote></blockquote>
</div><div class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
3) How to enable 'remarks'<br>
<br>
We need a way to enable 'remark' diagnostics. Quentin proposed to go<br>
for an approach similar to the warning flags. Where we control remarks<br>
with '-Rvector', '-Rloop-vector', ...<br>
<br>
I will read a little bit through the existing option system to better<br>
understand what it is doing, possibly adding documentation / cleanups<br>
on my way. I will come back with a proposal here.<br>
</blockquote>
<br>
It’s a bit odd, but since these are diagnostics, why not use the<br>
existing -W flags? You should be able to -Werror one of these,<br>
control them with #pragma clang diagnostics, etc. It doesn’t seem<br>
like we need more complexity in this space.<br>
</blockquote>
<br>
Good point. I will prepare the above patches such that they reuse the<br>
existing infrastructure. If we really see a need for further<br>
adjustments, we can do this incrementally.<br>
</div></blockquote>
<br></div></div><div class="">
I just updated the patch to reflect the conclusions of our discussion.<br>
Please review for commit.<br>
<br>
------------<br>
[PATCH] Add 'remark' diagnostic type in 'clang'<div><br>
<br>
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.<br>
<br>
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.<br>
<br></div>
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<br>
can be enabled with normal '-Wdiagnostic-name' flags and can be upgraded to an error using '-Werror=diagnostic-name'.<br></div></blockquote><div><br></div><div>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".</div>
<div>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.</div></div></div></div></blockquote><div><br></div><div>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?</div>
<div><br></div><div>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.</div>
<div><br></div><div>The patch itself looks good. Some quibbles:</div><div><br></div><div><div>--- a/include/clang/Basic/DiagnosticIDs.h</div><div>+++ b/include/clang/Basic/DiagnosticIDs.h</div><div> enum Mapping {</div>
<div> // NOTE: 0 means "uncomputed".</div><div> MAP_IGNORE = 1, ///< Map this diagnostic to nothing, ignore it.</div><div> MAP_WARNING = 2, ///< Map this diagnostic to a warning.</div>
<div> MAP_ERROR = 3, ///< Map this diagnostic to an error.</div><div>- MAP_FATAL = 4 ///< Map this diagnostic to a fatal error.</div><div>+ MAP_REMARK = 4, ///< Map this diagnostic to a remark.</div>
<div>+ MAP_FATAL = 5 ///< Map this diagnostic to a fatal error.</div><div> };</div></div><div><br></div><div>Why is REMARK here, rather than between IGNORE and WARNING?</div><div><br></div><div><br></div>
<div><div>--- a/lib/Frontend/TextDiagnostic.cpp</div><div>+++ b/lib/Frontend/TextDiagnostic.cpp</div><div>@@ -711,6 +713,7 @@ TextDiagnostic::printDiagnosticLevel(raw_ostream &OS,<br></div><div> case DiagnosticsEngine::Ignored:</div>
<div> llvm_unreachable("Invalid diagnostic type");</div><div> case DiagnosticsEngine::Note: OS.changeColor(noteColor, true); break;</div><div>+ case DiagnosticsEngine::Remark: OS.changeColor(remarkColor, true); break;</div>
<div> case DiagnosticsEngine::Warning: OS.changeColor(warningColor, true); break;</div><div> case DiagnosticsEngine::Error: OS.changeColor(errorColor, true); break;</div><div> case DiagnosticsEngine::Fatal: OS.changeColor(fatalColor, true); break;</div>
<div>@@ -721,6 +724,7 @@ TextDiagnostic::printDiagnosticLevel(raw_ostream &OS,</div><div> case DiagnosticsEngine::Ignored:</div><div> llvm_unreachable("Invalid diagnostic type");</div><div> case DiagnosticsEngine::Note: OS << "note"; break;</div>
<div>+ case DiagnosticsEngine::Remark: OS << "remark"; break;</div><div> case DiagnosticsEngine::Warning: OS << "warning"; break;</div><div> case DiagnosticsEngine::Error: OS << "error"; break;</div>
<div> case DiagnosticsEngine::Fatal: OS << "fatal error"; break;</div></div><div><br></div><div>In both cases, whitespace doesn't match the surrounding statements.</div><div><br></div><div><br></div>
<div>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.</div></div></div></div>