Hi Hal, Thanks for you feedbacks.
On Dec 17, 2013, at 3:53 PM, Hal Finkel <[email protected]> wrote: > Hi Quentin, > > Any reason you did not use phabricator for this patch? I thought it was trivial enough not to require a phabricator. > > +def BackendInlineAsm : DiagGroup<"backend-inline-asm">; > > +def BackendStackSize : DiagGroup<"backend-stack-size">; > > +def BackendPlugin : DiagGroup<"backend-plugin">; > > Are these group names user-visible? I think they are. At least, I get this name when a warning is issued: warning: stack size exceeded (168) in main [-Wbackend-stack-size] 1 warning generated. > If they are, do we want "backend" in the name? I do not have any strong opinion on that. This prefix makes it clear that these diagnostics are issued by the backend. I thought it may be useful to have a naming convention for those. > > + // 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 ;) This was part of the missing points, so if we can ,sure! The question is should we hold the patch on that... I really do not know. At first glance, it does not seem easy to demangle function name, but I am not familiar with clang’s internal and I may have miss something obvious. > -- 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. I disagree. In my opinion the diagnostic printer should be reserved for unknown diagnostic (what we called plugins). Indeed, as soon as a diagnostic is statically known, I prefer to have a special case in the frontend so what the localization burden is in there (e.g., for the stack size warning the message string is in the related td file). > > 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. You mean to demangle function name? -Quentin > > -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
