[cfe-commits] [PATCH] Refactor Diagnostic class

[Chris Lattner]

Excellent, thanks Argyrios!

-Chris

On Nov 18, 2010, at 4:28 PM, Argyrios Kyrtzidis wrote:

> On Nov 17, 2010, at 12:23 PM, Chris Lattner wrote:
> 
>> 
>> On Nov 15, 2010, at 3:25 PM, Argyrios Kyrtzidis wrote:
>> 
>>> -The attached patch separates Diagnostic class into 2 classes, Diagnostic and DiagnosticIDs.
>>> -Move the stuff of Diagnostic related to creating/querying diagnostic IDs into the new DiagnosticIDs class.
>>> -DiagnosticIDs can be shared among multiple Diagnostics for multiple translation units.
>>> -The rest of the state in Diagnostic object is considered related and tied to one translation unit.
>>> -Have Diagnostic point to the SourceManager that is related with. Diagnostic can now accept just a SourceLocation instead of a FullSourceLoc.
>>> -Reflect the changes to various interfaces.
>> 
>> Overall, this looks really great.  Some minor thoughts:
> 
> Applied at r119730. Thanks for reviewing!
> 
>> 
>> +    // Aggregate the number of warnings/errors from all diagnostics so
>> +    // that at CompilerInstance::ExecuteAction we can report the total numbers.
>> +    // FIXME: This is hacky, maybe keep track of total number of warnings/errors
>> +    // in DiagnosticClient and have CompilerInstance query that ?
>> +    CI.getDiagnostics().setNumWarnings(CI.getDiagnostics().getNumWarnings() +
>> +                                       Diags->getNumWarnings());
>> +    CI.getDiagnostics().setNumErrors(CI.getDiagnostics().getNumErrors() +
>> +                                     Diags->getNumErrors());
>> 
>> I agree with your fixme, the numerror/numwarning count should be moved into the diagnostic client, because you could conceptually have multiple different Diagnostic objects talking to one diagclient.  It seems likely that Diagnostic would still need a "had any error" method, though maybe even that could be factored out somehow.
> 
> Made DiagnosticClient to keep track of number of warnings/errors at r119731.
> After some commits later, I also removed Diagnostic's get/setNumErrors().
> get/setNumWarnings() is currently having one use by the preamble stuff but I'll look into whether this can be removed as well.
> 
> -Argiris
> 
>> 
>>      // If the diagnostic is an extension diagnostic and not enabled by default
>>      // then it must have been turned on with -pedantic.
>>      bool EnabledByDefault;
>> -      if (Diagnostic::isBuiltinExtensionDiag(Info.getID(), EnabledByDefault) &&
>> +      if (DiagnosticIDs::isBuiltinExtensionDiag(Info.getID(), EnabledByDefault) &&
>> 
>> Watch 80 columns.
>> 
>> Otherwise, looks great!
>> 
>> -Chris
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>