[cfe-commits] r159875 - in /cfe/trunk: include/clang/Basic/DiagnosticGroups.td include/clang/Driver/Options.td lib/Driver/Tools.cpp lib/Frontend/TextDiagnosticPrinter.cpp test/Driver/warning-options.cpp test/Misc/show-diag-options.c test/Misc/warning-flags.c tools/libclang/CXStoredDiagnostic.cpp utils/TableGen/ClangDiagnosticsEmitter.cpp

Ted Kremenek kremenek at apple.com
Sat Jul 7 00:25:06 PDT 2012


On Jul 6, 2012, at 11:37 PM, John McCall <rjmccall at apple.com> 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.




More information about the cfe-commits mailing list