[cfe-commits] [PATCH] Refactor Diagnostic class

[Chris Lattner]

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:

+    // 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.

       // 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