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? 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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
