On Wed, Jul 24, 2013 at 10:23 PM, Chris Lattner <clattner at apple.com> wrote:> On Jul 24, 2013, at 10:16 PM, David Blaikie <dblaikie at gmail.com> wrote: >>> How about this: keep the jist of the current API, but drop the "warning"- or >>> "error"-ness of the API. Instead, the backend just includes an enum value >>> (plus string message for extra data). The frontend makes the decision of >>> how to render the diagnostic (or not, dropping them is fine) along with how >>> to map them onto warning/error or whatever concepts they use. >> >> I'm not quite clear from your suggestion whether you're suggesting the >> backend would produce a complete diagnostic string, or just the >> parameters - requiring/leaving it up to the frontend to have a full >> textual string for each backend diagnostic with the right number of >> placeholders, etc. I'm sort of in two minds about that - I like the >> idea that frontends keep all the user-rendered text (means >> localization issues are in one place, the frontend - rather than >> ending up with english text backend diagnostics rendered in a >> non-english client (yeah, pretty hypothetical, I'm not sure anyone has >> localized uses of LLVM)). But this does mean that there's no "free >> ride" - frontends must have some explicit handling of each backend >> diagnostic (some crappy worst-case fallback, but it won't be a useful >> message). > > I don't have a specific proposal in mind, other than thinking along the exact same lines as you above. :) > > The best approach is probably hybrid: the diagnostic producer can produce *both* a full string like today, as well as an "ID + enum" pair. This way, clang can use the later, but llc (as one example of something we want to keep simple) could print the former, and frontends that get unknown enums could fall back on the full string.Fair-ish. If it were just for the sake of llc it'd be hard to justify having the strings in LLVM rather than just in llc itself, but providing them as fallbacks is probably reasonable/convenient & not likely to be a technical burden. Hopefully if we wanted that we'd still put something in Clang to maintain the frontend diagnostic line rather than letting it slip.> >> & I don't think this avoids the desire to have non-diagnostic >> callbacks whenever possible (notify of interesting things, frontends >> can decide whether to use that information to emit a diagnostic based >> on some criteria or behave differently in another way). > > Sure, but we also don't want to block progress in some area because we have a desire to solve a bigger problem.Sure enough - I think the only reason to pre-empt the bigger problem is to ensure that the immediate progress doesn't lead to bad implementations of those bigger issues being committed due to convenience. Ensuring that the solution we implement now makes it hard to justify (not hard to /do/ badly, just hard to justify doing it badly by ensuring that the right solution is convenient/easy) taking shortcuts later would be good. That might just be the difference between having a function pointer callback for the diagnostic case and instead having a callback type with the diagnostic callback as the first one, with the intent to add more in cases where backend-diagnostics aren't the right tool. That way we have an callback interface we can easily extend. (ideally I'd love to have two things in the callback interface early, as an example - but that's not necessary & probably won't happen) I don't know enough about the particular things Quentin's planning to implement to know whether any of them fall into the "probably shouldn't be an LLVM diagnostic" bag.
Hi, I think we have a consensus on how we should report diagnostics now. For broader uses, the discussion is still open. To move forward on the diagnostic part, here is the plan: - Extend the current handler with a prototype like: void report(enum Kind, enum Classification, const char* msg) where - Kind is the kind of report: InlineAsm, StackSize, Other. - Classification is Error, Warning. - msg contains the fall back message to print in case the front-end do not know what to do with the report. How does this sound? Thanks again for all the contributions made to this thread so far! Cheers, Quentin On Jul 24, 2013, at 10:30 PM, David Blaikie <dblaikie at gmail.com> wrote:> On Wed, Jul 24, 2013 at 10:23 PM, Chris Lattner <clattner at apple.com> wrote: >> On Jul 24, 2013, at 10:16 PM, David Blaikie <dblaikie at gmail.com> wrote: >>>> How about this: keep the jist of the current API, but drop the "warning"- or >>>> "error"-ness of the API. Instead, the backend just includes an enum value >>>> (plus string message for extra data). The frontend makes the decision of >>>> how to render the diagnostic (or not, dropping them is fine) along with how >>>> to map them onto warning/error or whatever concepts they use. >>> >>> I'm not quite clear from your suggestion whether you're suggesting the >>> backend would produce a complete diagnostic string, or just the >>> parameters - requiring/leaving it up to the frontend to have a full >>> textual string for each backend diagnostic with the right number of >>> placeholders, etc. I'm sort of in two minds about that - I like the >>> idea that frontends keep all the user-rendered text (means >>> localization issues are in one place, the frontend - rather than >>> ending up with english text backend diagnostics rendered in a >>> non-english client (yeah, pretty hypothetical, I'm not sure anyone has >>> localized uses of LLVM)). But this does mean that there's no "free >>> ride" - frontends must have some explicit handling of each backend >>> diagnostic (some crappy worst-case fallback, but it won't be a useful >>> message). >> >> I don't have a specific proposal in mind, other than thinking along the exact same lines as you above. :) >> >> The best approach is probably hybrid: the diagnostic producer can produce *both* a full string like today, as well as an "ID + enum" pair. This way, clang can use the later, but llc (as one example of something we want to keep simple) could print the former, and frontends that get unknown enums could fall back on the full string.Agreed. That is exactly what I tried to express.> > Fair-ish. If it were just for the sake of llc it'd be hard to justify > having the strings in LLVM rather than just in llc itself, but > providing them as fallbacks is probably reasonable/convenient & not > likely to be a technical burden. Hopefully if we wanted that we'd > still put something in Clang to maintain the frontend diagnostic line > rather than letting it slip. > >> >>> & I don't think this avoids the desire to have non-diagnostic >>> callbacks whenever possible (notify of interesting things, frontends >>> can decide whether to use that information to emit a diagnostic based >>> on some criteria or behave differently in another way). >> >> Sure, but we also don't want to block progress in some area because we have a desire to solve a bigger problem. > > Sure enough - I think the only reason to pre-empt the bigger problem > is to ensure that the immediate progress doesn't lead to bad > implementations of those bigger issues being committed due to > convenience. Ensuring that the solution we implement now makes it hard > to justify (not hard to /do/ badly, just hard to justify doing it > badly by ensuring that the right solution is convenient/easy) taking > shortcuts later would be good. > > That might just be the difference between having a function pointer > callback for the diagnostic case and instead having a callback type > with the diagnostic callback as the first one, with the intent to add > more in cases where backend-diagnostics aren't the right tool. That > way we have an callback interface we can easily extend. (ideally I'd > love to have two things in the callback interface early, as an example > - but that's not necessary & probably won't happen) > > I don't know enough about the particular things Quentin's planning to > implement to know whether any of them fall into the "probably > shouldn't be an LLVM diagnostic" bag.For now, I just want to provide a way for the back-end to express diagnostic.> > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130725/025382b1/attachment.html>
On Jul 25, 2013, at 5:09 PM, Quentin Colombet <qcolombet at apple.com> wrote:> Hi, > > I think we have a consensus on how we should report diagnostics now. > For broader uses, the discussion is still open. > > To move forward on the diagnostic part, here is the plan: > - Extend the current handler with a prototype like: > void report(enum Kind, enum Classification, const char* msg) > where > - Kind is the kind of report: InlineAsm, StackSize, Other. > - Classification is Error, Warning. > - msg contains the fall back message to print in case the front-end do not know what to do with the report. > > How does this sound?Sounds like the right direction, how about extending it a bit more to be:> void report(enum Kind, StringRef StringData, enum Classification, StringRef msg)The idea is that "StringData+Kind" can be used to format something nice in clang, but that "msg" fully covers it for clients that don't know the Kind enum. -Chris
On Jul 25, 2013, at 6:04 PM, David Blaikie <dblaikie at gmail.com> wrote:>>> void report(enum Kind, StringRef StringData, enum Classification, StringRef msg) >> >> The idea is that "StringData+Kind" can be used to format something nice in clang, but that "msg" fully covers it for clients that don't know the Kind enum. > > Presumably there are diagnostics that are going to have more than one > parameter, though. ArrayRef<StringRef>? (though this doesn't scale to > providing fancy things like DIVariable parameters, that would possibly > allow Clang to lookup the variable (or function, etc) & print out > cursors, etc)It's possible, but the full generality of any diagnostic can be folded (at some loss of generality) into a single string. To balance complexity and generality vs simplicity, "1" is a pretty decent number. -Chris -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130725/61166fd3/attachment.html>
Hi, On Jul 25, 2013, at 9:07 PM, Chris Lattner <clattner at apple.com> wrote:> On Jul 25, 2013, at 6:04 PM, David Blaikie <dblaikie at gmail.com> wrote: >>>> void report(enum Kind, StringRef StringData, enum Classification, StringRef msg) >>> >>> The idea is that "StringData+Kind" can be used to format something nice in clang, but that "msg" fully covers it for clients that don't know the Kind enum.Sounds good!>> >> Presumably there are diagnostics that are going to have more than one >> parameter, though. ArrayRef<StringRef>? (though this doesn't scale to >> providing fancy things like DIVariable parameters, that would possibly >> allow Clang to lookup the variable (or function, etc) & print out >> cursors, etc) > > It's possible, but the full generality of any diagnostic can be folded (at some loss of generality) into a single string. To balance complexity and generality vs simplicity, "1" is a pretty decent number.Agreed. Thanks for the help. Cheers, -Quentin -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130726/ecbbff90/attachment.html>
On 07/25/2013 05:09 PM, Quentin Colombet wrote:> Hi, > > I think we have a consensus on how we should report diagnostics now. > For broader uses, the discussion is still open. > > To move forward on the diagnostic part, here is the plan: > - Extend the current handler with a prototype like: > void report(enum Kind, enum Classification, const char* msg) > where > - Kind is the kind of report: InlineAsm, StackSize, Other. > - Classification is Error, Warning. > - msg contains the fall back message to print in case the front-end do not know what to do with the report.Hello Quentin, could you explain how plugins would use your infrastructure? Would it be possible for a plugin to provide (limited) warnings/diagnostics without requiring clang/LLVM to be adapted? Cheers, Tobias