On Jul 6, 2012, at 11:37 PM, John McCall <[email protected]> wrote:
> On Jul 6, 2012, at 4:07 PM, Ted Kremenek wrote: >> Modified: cfe/trunk/utils/TableGen/ClangDiagnosticsEmitter.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/utils/TableGen/ClangDiagnosticsEmitter.cpp?rev=159875&r1=159874&r2=159875&view=diff >> ============================================================================== >> --- cfe/trunk/utils/TableGen/ClangDiagnosticsEmitter.cpp (original) >> +++ cfe/trunk/utils/TableGen/ClangDiagnosticsEmitter.cpp Fri Jul 6 18:07:31 >> 2012 >> +class InferPedantic { >> + typedef llvm::DenseMap<const Record*, >> + std::pair<unsigned, llvm::Optional<unsigned> > > >> GMap; >> + >> + DiagGroupParentMap &DiagGroupParents; >> + const std::vector<Record*> &Diags; > > ArrayRef? > >> + const std::vector<Record*> DiagGroups; > > Missing a &, but probably should also be an ArrayRef. > >> + std::map<std::string, GroupInfo> &DiagsInGroup; > > StringMap? Most of this is just following the style of what was already in ClangDiagnosticsEmitter.cpp. The std::map, for example, was what was already being used, and I wasn't looking to face-lift the entire file in this patch (changing to a StringMap would impact other code beyond the change I was looking at). These are great critiques, however. > >> + /// Determine whether a group is a subgroup of another group. >> + bool isSubGroupOfGroup(const Record *Group, >> + llvm::StringRef RootGroupName); >> + >> + /// Determine if the diagnostic is an extension. >> + bool isExtension(const Record *Diag); >> + >> + /// Increment the count for a group, and transitively marked >> + /// parent groups when appropriate. >> + void markGroup(const Record *Group); >> + >> + /// Return true if the diagnostic is in a pedantic group. >> + bool groupInPedantic(const Record *Group, bool increment = false); > > Conventionally, this should be isGroupInPedantic. Makes sense. > >> +/// Determine if the diagnostic is an extension. >> +bool InferPedantic::isExtension(const Record *Diag) { >> + const std::string &ClsName = Diag->getValueAsDef("Class")->getName(); >> + return ClsName == "CLASS_EXTENSION"; >> +} > > Not also EXTWARN? This is correct. It's the same diagnostic class under the hood. See Diagnostics.td: class Extension<string str> : Diagnostic<str, CLASS_EXTENSION, MAP_IGNORE>; class ExtWarn<string str> : Diagnostic<str, CLASS_EXTENSION, MAP_WARNING>; > >> + // Consider a group in -Wpendatic IFF if has at least one diagnostic > > Typo: -Wpedantic > >> - >> + const bool IsPedantic = I->first == "pedantic"; > > We don't usually make locals const. Thanks for the review. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
