Hi Quentin, Any reason you did not use phabricator for this patch?
+def BackendInlineAsm : DiagGroup<"backend-inline-asm">; +def BackendStackSize : DiagGroup<"backend-stack-size">; +def BackendPlugin : DiagGroup<"backend-plugin">; Are these group names user-visible? If they are, do we want "backend" in the name? + // FIXME: We should demangle the function name. + // FIXME: Is there a way to get a location for that function? + FullSourceLoc Loc; + Diags.Report(Loc, diag::warn_fe_backend_stack_size) + << D.getStackSize() << D.getFunction().getName(); + return true; +} + We should figure this out ;) -- Furthermore, since you put in the work to have the diagnostic printers, we should use that to handle this stack-size warning instead of using this special class. Looking through CodeGenModule.cpp, it does not look like there is such a map currently, although it looks like we could maintain such a map in the CodeGenModule::GetOrCreateLLVMFunction function. -Hal ----- Original Message ----- > From: "Quentin Colombet" <[email protected]> > To: "Dmitri Gribenko" <[email protected]> > Cc: "llvm cfe" <[email protected]> > Sent: Tuesday, December 17, 2013 5:03:36 PM > Subject: Re: [PATCH] Add diagnostic capabilities in LLVM (frontend part) > > > > Hi Dimitri, > > Thanks for the quick reply! > > New patch attached. > > On Dec 17, 2013, at 11:58 AM, Dmitri Gribenko <[email protected]> > wrote: > > > On Tue, Dec 17, 2013 at 11:22 AM, Quentin Colombet > > <[email protected]> wrote: > >> - Tests. > >> I made some tests locally, but the patch does not add any test > >> case. The > >> question is how can we add tests that require both a backend and a > >> frontend > >> in either the frontend or the backend? > > > > A lot of tests in Clang have "REQUIRES: x86-registered-target" -- > > please take a look at them. > Thanks, using your advice, I have added a test in > test/Frontend/backend-diagnostic.c. > > > > > About the patch: > > > > 80 columns. > Oh sorry about that, I was sure I had run clang-format… apparently > not! > Fixed. > > > > > +/// DiagnosticHandler2 - This function is invoked when the backend > > needs > > +/// to report something to the user. > > > > Please don't duplicate function name in comments. > Fixed. > > > > > +/// The \p DI interface provides two basic information: > > +/// getKind() helps to determines what it is reporting and > > +/// getSeverity() decribes how bad this is. > > > > This does not really belong to the documentation for > > DiagnosticHandler2(), I think. > Removed. > > > > > +#define SeveritySwitch(Severity, GroupName, DiagID) > > \ > > + switch (Severity) { > > \ > > > > Please wrap this in do {...} while(false), so that the trailing > > semicolon in the macro invocation gets correctly attached to the > > expansion. > Done. > > > > > Also, this macro could have a better name -- maybe ComputeSeverity? > Renamed into ComputeDiagID. > > Thanks, > -Quentin > > > > > > > > Dmitri > > > > -- > > main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if > > (j){printf("%d\n",i);}}} /*Dmitri Gribenko <[email protected]>*/ > > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > -- Hal Finkel Assistant Computational Scientist Leadership Computing Facility Argonne National Laboratory _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
