[cfe-commits] [PATCH] Refactor Diagnostic class

[Argyrios Kyrtzidis]

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