Eric Fiselier <[email protected]> writes: > Responding to @bogner's comments that he sent via email. > > > ================ > Comment at: include/clang/Basic/DiagnosticOptions.h:37 > @@ -27,1 +36,3 @@ > +}; > + > /// \brief Options for controlling the compiler diagnostics engine. > ---------------- >> Better to use `enum class DiagnosticLevelMask`. You'll need to define >> operator| and operator|=, but those are trivial with std::underlying_type. > > I would add `operator&(Enum, Enum)`, `operator|(Enum, Enum)` and `~operator( > > I would prefer to use a enum class but there are a couple of problems: > 1. You need to define a `raw_ostream operator<<(...)` function. This > is required by the ENUM_DIAGOPT macro.
This is a little lame, sure. We could always use some templates and SFINAE to reduce the boilerplate if we start using this for more things though - ie, adding an enable_if'd operator<< to raw_ostream for enums where we just want to print the underlying type. I don't think this is really necessary at this point though. Just an idea. > 2. `if (EnumValue & Enum::Value)` doesn't work. You need the explicit > bool cast which is ugly. This one's harder to improve - you either get type safety or convenient bools, I guess. I personally like the enum class version you have here better, but I could see the argument for just using a raw enum if you prefer that. Either way's fine. Feel free to commit once you've addressed the last couple of cosmetic issues below. > > ================ > Comment at: lib/Frontend/VerifyDiagnosticConsumer.cpp:750 > @@ -747,1 +749,3 @@ > > + DiagnosticLevelMask const IgnoredUnexpectedLevels = > + Diags.getDiagnosticOptions().VerifyIgnoreUnexpected; > ---------------- >> Why bother making this const? Also, a name like DiagMask is probably clearer. > > I like to make things const so I can't accidentally change it. I'm > happy to change the name though. Alright - I think the prevalent style in LLVM is "const DiagnosticLevelMask" rather than "DiagnosticLevelMask const" though. Please write it that way for consistency. > http://reviews.llvm.org/D10138 > > EMAIL PREFERENCES > http://reviews.llvm.org/settings/panel/emailpreferences/ Eric Fiselier <[email protected]> writes: > Address bogner's comments. > > > http://reviews.llvm.org/D10138 > > Files: > include/clang/Basic/DiagnosticOptions.def > include/clang/Basic/DiagnosticOptions.h > include/clang/Driver/CC1Options.td > lib/Basic/CMakeLists.txt > lib/Basic/DiagnosticOptions.cpp > lib/Frontend/CompilerInvocation.cpp > lib/Frontend/VerifyDiagnosticConsumer.cpp > test/Frontend/verify-ignore-unexpected.c > > EMAIL PREFERENCES > http://reviews.llvm.org/settings/panel/emailpreferences/ > Index: include/clang/Basic/DiagnosticOptions.def > =================================================================== > --- include/clang/Basic/DiagnosticOptions.def > +++ include/clang/Basic/DiagnosticOptions.def > @@ -69,7 +69,10 @@ > DIAGOPT(VerifyDiagnostics, 1, 0) /// Check that diagnostics match the > expected > /// diagnostics, indicated by markers in the > /// input source file. > - > +ENUM_DIAGOPT(VerifyIgnoreUnexpected, DiagnosticLevelMask, 4, > + DiagnosticLevelMask::None) /// Ignore unexpected diagnostics of > + /// the specified levels when using > + /// -verify. > DIAGOPT(ElideType, 1, 0) /// Elide identical types in template > diffing > DIAGOPT(ShowTemplateTree, 1, 0) /// Print a template tree when diffing > DIAGOPT(CLFallbackMode, 1, 0) /// Format for clang-cl fallback mode > Index: include/clang/Basic/DiagnosticOptions.h > =================================================================== > --- include/clang/Basic/DiagnosticOptions.h > +++ include/clang/Basic/DiagnosticOptions.h > @@ -24,6 +24,35 @@ > Ovl_Best ///< Show just the "best" overload candidates. > }; > > +/// \brief A bitmask representing the diagnostic levels used by > +/// VerifyDiagnosticConsumer. > +enum class DiagnosticLevelMask : unsigned { > + None = 0, > + Note = 1 << 0, > + Remark = 1 << 1, > + Warning = 1 << 2, > + Error = 1 << 3, > + All = Note | Remark | Warning | Error > +}; > + > +inline DiagnosticLevelMask operator~(DiagnosticLevelMask M) { > + return static_cast<DiagnosticLevelMask>(~static_cast<unsigned>(M)); > +} It doesn't make a difference here since it'll probably never change, but I prefer to use std::underlying_type for these, like: inline DiagnosticLevelMask operator~(DiagnosticLevelMask M) { using UT = std::underlying_type<DiagnosticLevelMask>::type; return static_cast<DiagnosticLevelMask>(~static_cast<UT>(M)); } If you disagree, the current way is fine. > + > +inline DiagnosticLevelMask operator|(DiagnosticLevelMask LHS, > + DiagnosticLevelMask RHS) { > + return static_cast<DiagnosticLevelMask>( > + static_cast<unsigned>(LHS) | static_cast<unsigned>(RHS)); > +} > + > +inline DiagnosticLevelMask operator&(DiagnosticLevelMask LHS, > + DiagnosticLevelMask RHS) { > + return static_cast<DiagnosticLevelMask>( > + static_cast<unsigned>(LHS) & static_cast<unsigned>(RHS)); > +} > + > +raw_ostream& operator<<(raw_ostream& Out, DiagnosticLevelMask M); > + > /// \brief Options for controlling the compiler diagnostics engine. > class DiagnosticOptions : public RefCountedBase<DiagnosticOptions>{ > public: > Index: include/clang/Driver/CC1Options.td > =================================================================== > --- include/clang/Driver/CC1Options.td > +++ include/clang/Driver/CC1Options.td > @@ -301,6 +301,10 @@ > HelpText<"Format message diagnostics so that they fit within N columns or > fewer, when possible.">; > def verify : Flag<["-"], "verify">, > HelpText<"Verify diagnostic output using comment directives">; > +def verify_ignore_unexpected : Flag<["-"], "verify-ignore-unexpected">, > + HelpText<"Ignore unexpected diagnostic messages">; > +def verify_ignore_unexpected_EQ : CommaJoined<["-"], > "verify-ignore-unexpected=">, > + HelpText<"Ignore unexpected diagnostic messages">; > def Wno_rewrite_macros : Flag<["-"], "Wno-rewrite-macros">, > HelpText<"Silence ObjC rewriting warnings">; > > Index: lib/Basic/CMakeLists.txt > =================================================================== > --- lib/Basic/CMakeLists.txt > +++ lib/Basic/CMakeLists.txt > @@ -61,6 +61,7 @@ > CharInfo.cpp > Diagnostic.cpp > DiagnosticIDs.cpp > + DiagnosticOptions.cpp > FileManager.cpp > FileSystemStatCache.cpp > IdentifierTable.cpp > Index: lib/Basic/DiagnosticOptions.cpp > =================================================================== > --- /dev/null > +++ lib/Basic/DiagnosticOptions.cpp > @@ -0,0 +1,23 @@ > +//===--- DiagnosticOptions.cpp - C Language Family Diagnostic Handling > ----===// > +// > +// The LLVM Compiler Infrastructure > +// > +// This file is distributed under the University of Illinois Open Source > +// License. See LICENSE.TXT for details. > +// > +//===----------------------------------------------------------------------===// > +// > +// This file implements the DiagnosticOptions related interfaces. > +// > +//===----------------------------------------------------------------------===// > + > +#include "clang/Basic/DiagnosticOptions.h" > +#include "llvm/Support/raw_ostream.h" > + > +namespace clang { > + > +raw_ostream& operator<<(raw_ostream& Out, DiagnosticLevelMask M) { > + return Out << static_cast<unsigned>(M); > +} > + > +} // end namespace clang > Index: lib/Frontend/CompilerInvocation.cpp > =================================================================== > --- lib/Frontend/CompilerInvocation.cpp > +++ lib/Frontend/CompilerInvocation.cpp > @@ -321,6 +321,29 @@ > return Pattern; > } > > +static bool parseDiagnosticLevelMask(StringRef FlagName, > + const std::vector<std::string> &Levels, > + DiagnosticsEngine *Diags, > + DiagnosticLevelMask &M) { > + bool Success = true; > + for (const auto &Level : Levels) { > + DiagnosticLevelMask const PM = > + llvm::StringSwitch<DiagnosticLevelMask>(Level) > + .Case("note", DiagnosticLevelMask::Note) > + .Case("remark", DiagnosticLevelMask::Remark) > + .Case("warning", DiagnosticLevelMask::Warning) > + .Case("error", DiagnosticLevelMask::Error) > + .Default(DiagnosticLevelMask::None); > + if (PM == DiagnosticLevelMask::None) { > + Success = false; > + if (Diags) > + Diags->Report(diag::err_drv_invalid_value) << FlagName << Level; > + } > + M = M | PM; > + } > + return Success; > +} > + > static void parseSanitizerKinds(StringRef FlagName, > const std::vector<std::string> &Sanitizers, > DiagnosticsEngine &Diags, SanitizerSet &S) { > @@ -750,11 +773,18 @@ > << Args.getLastArg(OPT_fdiagnostics_format)->getAsString(Args) > << Format; > } > - > + > Opts.ShowSourceRanges = > Args.hasArg(OPT_fdiagnostics_print_source_range_info); > Opts.ShowParseableFixits = Args.hasArg(OPT_fdiagnostics_parseable_fixits); > Opts.ShowPresumedLoc = > !Args.hasArg(OPT_fno_diagnostics_use_presumed_location); > Opts.VerifyDiagnostics = Args.hasArg(OPT_verify); > + DiagnosticLevelMask DiagMask = DiagnosticLevelMask::None; > + Success &= parseDiagnosticLevelMask("-verify-ignore-unexpected=", > + Args.getAllArgValues(OPT_verify_ignore_unexpected_EQ), > + Diags, DiagMask); > + if (Args.hasArg(OPT_verify_ignore_unexpected)) > + DiagMask = DiagnosticLevelMask::All; > + Opts.setVerifyIgnoreUnexpected(DiagMask); > Opts.ElideType = !Args.hasArg(OPT_fno_elide_type); > Opts.ShowTemplateTree = Args.hasArg(OPT_fdiagnostics_show_template_tree); > Opts.ErrorLimit = getLastArgIntValue(Args, OPT_ferror_limit, 0, Diags); > Index: lib/Frontend/VerifyDiagnosticConsumer.cpp > =================================================================== > --- lib/Frontend/VerifyDiagnosticConsumer.cpp > +++ lib/Frontend/VerifyDiagnosticConsumer.cpp > @@ -691,7 +691,8 @@ > const char *Label, > DirectiveList &Left, > const_diag_iterator d2_begin, > - const_diag_iterator d2_end) { > + const_diag_iterator d2_end, > + bool IgnoreUnexpected) { > std::vector<Directive *> LeftOnly; > DiagList Right(d2_begin, d2_end); > > @@ -727,7 +728,8 @@ > } > // Now all that's left in Right are those that were not matched. > unsigned num = PrintExpected(Diags, SourceMgr, LeftOnly, Label); > - num += PrintUnexpected(Diags, &SourceMgr, Right.begin(), Right.end(), > Label); > + if (!IgnoreUnexpected) > + num += PrintUnexpected(Diags, &SourceMgr, Right.begin(), Right.end(), > Label); > return num; > } > > @@ -745,21 +747,28 @@ > // Seen \ Expected - set seen but not expected > unsigned NumProblems = 0; > > + DiagnosticLevelMask const DiagMask = > + Diags.getDiagnosticOptions().getVerifyIgnoreUnexpected(); > + > // See if there are error mismatches. > NumProblems += CheckLists(Diags, SourceMgr, "error", ED.Errors, > - Buffer.err_begin(), Buffer.err_end()); > + Buffer.err_begin(), Buffer.err_end(), > + bool(DiagnosticLevelMask::Error & DiagMask)); > > // See if there are warning mismatches. > NumProblems += CheckLists(Diags, SourceMgr, "warning", ED.Warnings, > - Buffer.warn_begin(), Buffer.warn_end()); > + Buffer.warn_begin(), Buffer.warn_end(), > + bool(DiagnosticLevelMask::Warning & DiagMask)); > > // See if there are remark mismatches. > NumProblems += CheckLists(Diags, SourceMgr, "remark", ED.Remarks, > - Buffer.remark_begin(), Buffer.remark_end()); > + Buffer.remark_begin(), Buffer.remark_end(), > + bool(DiagnosticLevelMask::Remark & DiagMask)); > > // See if there are note mismatches. > NumProblems += CheckLists(Diags, SourceMgr, "note", ED.Notes, > - Buffer.note_begin(), Buffer.note_end()); > + Buffer.note_begin(), Buffer.note_end(), > + bool(DiagnosticLevelMask::Note & DiagMask)); > > return NumProblems; > } > @@ -854,12 +863,20 @@ > // Check that the expected diagnostics occurred. > NumErrors += CheckResults(Diags, *SrcManager, *Buffer, ED); > } else { > - NumErrors += (PrintUnexpected(Diags, nullptr, Buffer->err_begin(), > - Buffer->err_end(), "error") + > - PrintUnexpected(Diags, nullptr, Buffer->warn_begin(), > - Buffer->warn_end(), "warn") + > - PrintUnexpected(Diags, nullptr, Buffer->note_begin(), > - Buffer->note_end(), "note")); > + DiagnosticLevelMask const DiagMask = > + ~Diags.getDiagnosticOptions().getVerifyIgnoreUnexpected(); > + if (bool(DiagnosticLevelMask::Error & DiagMask)) > + NumErrors += PrintUnexpected(Diags, nullptr, Buffer->err_begin(), > + Buffer->err_end(), "error"); > + if (bool(DiagnosticLevelMask::Warning & DiagMask)) > + NumErrors += PrintUnexpected(Diags, nullptr, Buffer->warn_begin(), > + Buffer->warn_end(), "warn"); > + if (bool(DiagnosticLevelMask::Remark & DiagMask)) > + NumErrors += PrintUnexpected(Diags, nullptr, Buffer->remark_begin(), > + Buffer->remark_end(), "remark"); > + if (bool(DiagnosticLevelMask::Note & DiagMask)) > + NumErrors += PrintUnexpected(Diags, nullptr, Buffer->note_begin(), > + Buffer->note_end(), "note"); > } > > Diags.setClient(CurClient, Owner.release() != nullptr); > Index: test/Frontend/verify-ignore-unexpected.c > =================================================================== > --- /dev/null > +++ test/Frontend/verify-ignore-unexpected.c > @@ -0,0 +1,81 @@ > +// RUN: not %clang_cc1 -DTEST_SWITCH > -verify-ignore-unexpected=remark,aoeu,note -verify %s 2>&1 \ > +// RUN: | FileCheck -check-prefix=CHECK-BAD-SWITCH %s > +#ifdef TEST_SWITCH > +// expected-no-diagnostics > +#endif > +// CHECK-BAD-SWITCH: error: 'error' diagnostics seen but not expected: > +// CHECK-BAD-SWITCH-NEXT: (frontend): invalid value 'aoeu' in > '-verify-ignore-unexpected=' > + > +// RUN: %clang_cc1 -DTEST1 -verify %s > +// RUN: %clang_cc1 -DTEST1 -verify -verify-ignore-unexpected %s > +#ifdef TEST1 > +#warning MyWarning1 > + // expected-warning@-1 {{MyWarning1}} > +int x; // expected-note {{previous definition is here}} > +float x; // expected-error {{redefinition of 'x'}} > +#endif > + > +// RUN: not %clang_cc1 -DTEST2 -verify %s 2>&1 \ > +// RUN: | FileCheck -check-prefix=CHECK-UNEXP %s > +// RUN: not %clang_cc1 -DTEST2 -verify -verify-ignore-unexpected= %s 2>&1 \ > +// RUN: | FileCheck -check-prefix=CHECK-UNEXP %s > +// RUN: not %clang_cc1 -DTEST2 -verify -verify-ignore-unexpected=note %s > 2>&1 \ > +// RUN: | FileCheck -check-prefix=CHECK-NOTE %s > +// RUN: not %clang_cc1 -DTEST2 -verify -verify-ignore-unexpected=warning %s > 2>&1 \ > +// RUN: | FileCheck -check-prefix=CHECK-WARN %s > +// RUN: not %clang_cc1 -DTEST2 -verify -verify-ignore-unexpected=error %s > 2>&1 \ > +// RUN: | FileCheck -check-prefix=CHECK-ERR %s > +#ifdef TEST2 > +#warning MyWarning2 > +int x; > +float x; > +#endif > +// CHECK-UNEXP: no expected directives found > +// CHECK-UNEXP-NEXT: 'error' diagnostics seen but not expected > +// CHECK-UNEXP-NEXT: Line {{[0-9]+}}: redefinition of 'x' > +// CHECK-UNEXP-NEXT: 'warning' diagnostics seen but not expected > +// CHECK-UNEXP-NEXT: Line {{[0-9]+}}: MyWarning2 > +// CHECK-UNEXP-NEXT: 'note' diagnostics seen but not expected > +// CHECK-UNEXP-NEXT: Line {{[0-9]+}}: previous definition is here > +// CHECK-UNEXP-NEXT: 4 errors generated. > + > +// CHECK-NOTE: no expected directives found > +// CHECK-NOTE-NEXT: 'error' diagnostics seen but not expected > +// CHECK-NOTE-NEXT: Line {{[0-9]+}}: redefinition of 'x' > +// CHECK-NOTE-NEXT: 'warning' diagnostics seen but not expected > +// CHECK-NOTE-NEXT: Line {{[0-9]+}}: MyWarning2 > +// CHECK-NOTE-NEXT: 3 errors generated. > + > +// CHECK-WARN: no expected directives found > +// CHECK-WARN-NEXT: 'error' diagnostics seen but not expected > +// CHECK-WARN-NEXT: Line {{[0-9]+}}: redefinition of 'x' > +// CHECK-WARN-NEXT: 'note' diagnostics seen but not expected > +// CHECK-WARN-NEXT: Line {{[0-9]+}}: previous definition is here > +// CHECK-WARN-NEXT: 3 errors generated. > + > +// CHECK-ERR: no expected directives found > +// CHECK-ERR-NEXT: 'warning' diagnostics seen but not expected > +// CHECK-ERR-NEXT: Line {{[0-9]+}}: MyWarning2 > +// CHECK-ERR-NEXT: 'note' diagnostics seen but not expected > +// CHECK-ERR-NEXT: Line {{[0-9]+}}: previous definition is here > +// CHECK-ERR-NEXT: 3 errors generated. > + > +// RUN: not %clang_cc1 -DTEST3 -verify -verify-ignore-unexpected %s 2>&1 \ > +// RUN: | FileCheck -check-prefix=CHECK-EXP %s > +#ifdef TEST3 > +// expected-error {{test3}} > +#endif > +// CHECK-EXP: 'error' diagnostics expected but not seen > +// CHECK-EXP-NEXT: Line {{[0-9]+}}: test3 > + > +// RUN: not %clang_cc1 -DTEST4 -verify -verify-ignore-unexpected %s 2>&1 \ > +// RUN: | FileCheck -check-prefix=CHECK-NOEXP %s > +// RUN: not %clang_cc1 -DTEST4 -verify > -verify-ignore-unexpected=warning,error,note %s 2>&1 \ > +// RUN: | FileCheck -check-prefix=CHECK-NOEXP %s > +#ifdef TEST4 > +#warning MyWarning4 > +int x; > +float x; > +#endif > +// CHECK-NOEXP: error: no expected directives found > +// CHECK-NOEXP-NEXT: 1 error generated _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
