<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Hi Hal,<div><br></div><div>Thanks for you feedbacks.<br><div><br><div><div>On Dec 17, 2013, at 3:53 PM, Hal Finkel <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Hi Quentin,<br><br>Any reason you did not use phabricator for this patch?<br></blockquote><div>I thought it was trivial enough not to require a phabricator.</div><br><blockquote type="cite"><br>+def BackendInlineAsm : DiagGroup<"backend-inline-asm">;<br><br>+def BackendStackSize : DiagGroup<"backend-stack-size">;<br><br>+def BackendPlugin : DiagGroup<"backend-plugin">;<br><br>Are these group names user-visible?</blockquote><div>I think they are. At least, I get this name when a warning is issued:</div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;"><span style="color: #d53bd3"><b>warning: </b></span><b>stack size exceeded (168) in main [-Wbackend-stack-size]</b></div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">1 warning generated.</div></div><br><blockquote type="cite"> If they are, do we want "backend" in the name?<br></blockquote><div>I do not have any strong opinion on that.</div><div>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.</div><br><blockquote type="cite"><br>+ // FIXME: We should demangle the function name.<br><br>+ // FIXME: Is there a way to get a location for that function?<br><br>+ FullSourceLoc Loc;<br><br>+ Diags.Report(Loc, diag::warn_fe_backend_stack_size)<br><br>+ << D.getStackSize() << D.getFunction().getName();<br><br>+ return true;<br><br>+}<br><br>+<br><br>We should figure this out ;)</blockquote><div>This was part of the missing points, so if we can ,sure!</div><div>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.</div><br><blockquote type="cite"> -- 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.<br></blockquote>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).</div><div><br><blockquote type="cite"><br>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.<br></blockquote><div>You mean to demangle function name?</div><div><br></div><div>-Quentin</div><br><blockquote type="cite"><br> -Hal<br><br>----- Original Message -----<br><blockquote type="cite">From: "Quentin Colombet" <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>><br>To: "Dmitri Gribenko" <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>><br>Cc: "llvm cfe" <<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a>><br>Sent: Tuesday, December 17, 2013 5:03:36 PM<br>Subject: Re: [PATCH] Add diagnostic capabilities in LLVM (frontend part)<br><br><br><br>Hi Dimitri,<br><br>Thanks for the quick reply!<br><br>New patch attached.<br><br>On Dec 17, 2013, at 11:58 AM, Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>><br>wrote:<br><br><blockquote type="cite">On Tue, Dec 17, 2013 at 11:22 AM, Quentin Colombet<br><<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:<br><blockquote type="cite">- Tests.<br>I made some tests locally, but the patch does not add any test<br>case. The<br>question is how can we add tests that require both a backend and a<br>frontend<br>in either the frontend or the backend?<br></blockquote><br>A lot of tests in Clang have "REQUIRES: x86-registered-target" --<br>please take a look at them.<br></blockquote>Thanks, using your advice, I have added a test in<br>test/Frontend/backend-diagnostic.c.<br><br><blockquote type="cite"><br>About the patch:<br><br>80 columns.<br></blockquote>Oh sorry about that, I was sure I had run clang-format… apparently<br>not!<br>Fixed.<br><br><blockquote type="cite"><br>+/// DiagnosticHandler2 - This function is invoked when the backend<br>needs<br>+/// to report something to the user.<br><br>Please don't duplicate function name in comments.<br></blockquote>Fixed.<br><br><blockquote type="cite"><br>+/// The \p DI interface provides two basic information:<br>+/// getKind() helps to determines what it is reporting and<br>+/// getSeverity() decribes how bad this is.<br><br>This does not really belong to the documentation for<br>DiagnosticHandler2(), I think.<br></blockquote>Removed.<br><br><blockquote type="cite"><br>+#define SeveritySwitch(Severity, GroupName, DiagID)<br>\<br>+ switch (Severity) {<br>\<br><br>Please wrap this in do {...} while(false), so that the trailing<br>semicolon in the macro invocation gets correctly attached to the<br>expansion.<br></blockquote>Done.<br><br><blockquote type="cite"><br>Also, this macro could have a better name -- maybe ComputeSeverity?<br></blockquote>Renamed into ComputeDiagID.<br><br>Thanks,<br>-Quentin<br><br><br><br><br><blockquote type="cite"><br>Dmitri<br><br>--<br>main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if<br>(j){printf("%d\n",i);}}} /*Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>>*/<br></blockquote><br><br><br>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits<br><br></blockquote><br>-- <br>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<br></blockquote></div><br></div></div></body></html>