| Message ID | 1539359025-10029-1-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCH/RFC] Add "User Experience Guidelines" to gccint.texi | expand |
On 10/12/18, David Malcolm <dmalcolm@redhat.com> wrote: > Here's a proposed "User Experience Guidelines" section for our > internals manual > > It's a mixture of proposed policy, together with notes on how to > implement the recommendations. > > Thoughts? I have no comments on the actual contents of the patch, just that it's a good idea to have such a section in general, and I hope it's approved! Eric > > gcc/ChangeLog: > * Makefile.in (TEXI_GCCINT_FILES): Add ux.texi. > * doc/gccint.texi: Include ux.texi and use it in top-level menu. > * doc/ux.texi: New file. > --- > gcc/Makefile.in | 2 +- > gcc/doc/gccint.texi | 2 + > gcc/doc/ux.texi | 455 > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 458 insertions(+), 1 deletion(-) > create mode 100644 gcc/doc/ux.texi > > diff --git a/gcc/Makefile.in b/gcc/Makefile.in > index 70efab7..3f05e95 100644 > --- a/gcc/Makefile.in > +++ b/gcc/Makefile.in > @@ -3176,7 +3176,7 @@ TEXI_GCCINT_FILES = gccint.texi gcc-common.texi > gcc-vers.texi \ > gnu.texi gpl_v3.texi fdl.texi contrib.texi languages.texi \ > sourcebuild.texi gty.texi libgcc.texi cfg.texi tree-ssa.texi \ > loop.texi generic.texi gimple.texi plugins.texi optinfo.texi \ > - match-and-simplify.texi poly-int.texi > + match-and-simplify.texi ux.texi poly-int.texi > > TEXI_GCCINSTALL_FILES = install.texi install-old.texi fdl.texi \ > gcc-common.texi gcc-vers.texi > diff --git a/gcc/doc/gccint.texi b/gcc/doc/gccint.texi > index 1a1af41..2554b31 100644 > --- a/gcc/doc/gccint.texi > +++ b/gcc/doc/gccint.texi > @@ -125,6 +125,7 @@ Additional tutorial information is linked to from > * LTO:: Using Link-Time Optimization. > > * Match and Simplify:: How to write expression simplification patterns for > GIMPLE and GENERIC > +* User Experience Guidelines:: Guidelines for implementing diagnostics and > options. > * Funding:: How to help assure funding for free software. > * GNU Project:: The GNU Project and GNU/Linux. > > @@ -162,6 +163,7 @@ Additional tutorial information is linked to from > @include plugins.texi > @include lto.texi > @include match-and-simplify.texi > +@include ux.texi > > @include funding.texi > @include gnu.texi > diff --git a/gcc/doc/ux.texi b/gcc/doc/ux.texi > new file mode 100644 > index 0000000..87ff599 > --- /dev/null > +++ b/gcc/doc/ux.texi > @@ -0,0 +1,455 @@ > +@c Copyright (C) 2018 Free Software Foundation, Inc. > +@c Free Software Foundation, Inc. > +@c This is part of the GCC manual. > +@c For copying conditions, see the file gcc.texi. > + > +@node User Experience Guidelines > +@chapter User Experience Guidelines > +@cindex User Experience Guidelines > +@cindex Guidelines, User Experience > + > +To borrow a slogan from > + @uref{https://elm-lang.org/blog/compilers-as-assistants, Elm}, > + > +@quotation > +@strong{Compilers should be assistants, not adversaries.} A compiler > should > +not just detect bugs, it should then help you understand why there is a > bug. > +It should not berate you in a robot voice, it should give you specific > hints > +that help you write better code. Ultimately, a compiler should make > +programming faster and more fun! > +@author Evan Czaplicki > +@end quotation > + > +This chapter provides guidelines on how to implement diagnostics and > +command-line options in ways that we hope achieve the above ideal. > + > +@menu > +* Guidelines for Diagnostics:: How to implement diagnostics. > +* Guidelines for Options:: Guidelines for command-line options. > +@end menu > + > + > +@node Guidelines for Diagnostics > +@cindex Guidelines for Diagnostics > +@cindex Diagnostics, Guidelines for > + > +@section Guidelines for Diagnostics > + > +@subsection Talk in terms of the user's code > + > +Diagnostics should be worded in terms of the user's source code, and the > +source language, rather than GCC's own implementation details. > + > +@subsection Diagnostics are ``actionable'' > + > +A good diagnostic is @emph{actionable}: it should assist the user in > +taking action. > + > +Consider what an end-user will want to do when encountering a diagnostic. > + > +Given an error, an end-user will think: ``How do I fix this?'' > + > +Given a warning, an end-user will want to review the warning and think: > + > +@itemize @bullet > +@item > +``Is this a ``true'' result?'' > +@item > +``Do I care?'' > +@item > +if they decide it's genuine: ``How do I fix this?'' > +@end itemize > + > +A good diagnostic provides pertinent information to allow the user to > +easily answer the above questions. > + > +@subsection The user's attention is important > + > +A perfect compiler would issue a warning on every aspect of the user's > +source code that ought to be fixed, and issue no other warnings. > +Naturally, this ideal is impossible to achieve. > + > +Warnings should have a good ``signal:noise ratio'': we should have few > +``false positives'' (falsely issuing a warning when no warning is > +warranted) and few ``false negatives'' (failing to issue a warning when > +one @emph{is} justified). > + > +Note that a ``false positive'' can mean, in practice, a warning that the > +user doesn't agree with. Ideally a diagnostic should contain enough > +information to allow the user to make an informed choice about whether > +they should care (and how to fix it), but a balance must be drawn against > +overloading the user with irrelevant data. > + > +It's worth testing a new warning on many instances of real-world code, > +written by different people, and seeing what it complains about, and > +what it doesn't complain about. This may suggest heuristics that > +silence common false positives. > + > +@subsection Make mismatches clear > + > +Many diagnostics relate to a mismatch between two different places in the > +user's source code. Examples include: > +@itemize @bullet > + @item > + a type mismatch, where the type at a usage site does not match the type > + at a declaration > + > + @item > + the argument count at a call site does not match the parameter count > + at the declaration > + > + @item > + something is erroneously duplicated (e.g. an error, due to breaking a > + uniqueness requirement, or a warning, if it's suggestive of a bug) > + > + @item > + an ``opened'' syntactic construct (such as an open-parenthesis) is not > + closed > + > + @c TODO: more examples? > +@end itemize > + > +In each case, the diagnostic should indicate @strong{both} pertinent > +locations (so that the user can easily see the problem and how to fix it). > + > +The standard way to do this is with a note (via @code{inform}). For > +example: > + > +@smallexample > + auto_diagnostic_group d; > + if (warning_at (loc, OPT_Wduplicated_cond, > + "duplicated %<if%> condition")) > + inform (EXPR_LOCATION (t), "previously used here"); > +@end smallexample > + > +For cases involving punctuation where the locations might be near > +each other, they can be conditionally consolidated via > +@code{gcc_rich_location::add_location_if_nearby}: > + > +@smallexample > + auto_diagnostic_group d; > + gcc_rich_location richloc (primary_loc); > + bool added secondary = richloc.add_location_if_nearby (secondary_loc); > + error_at (&richloc, "main message"); > + if (!added secondary) > + inform (secondary_loc, "message for secondary"); > +@end smallexample > + > +This will emit either one diagnostic with two locations: > +@smallexample > + demo.c:42:10: error: main message > + (foo) > + ~ ^ > +@end smallexample > + > +or two diagnostics: > + > +@smallexample > + demo.c:42:4: error: main message > + foo) > + ^ > + demo.c:40:2: note: message for secondary > + ( > + ^ > +@end smallexample > + > +@subsection Location Information > + > +GCC's @code{location_t} type can support both ``ordinary'' locations, > +and locations relating to a macro expansion. > + > +As of GCC 6, ordinary locations changed from supporting just a > +point in the user's source code to supporting three points: the > +``caret'' location, plus a start and a finish: > + > +@smallexample > + a = foo && bar; > + ~~~~^~~~~~ > + | | | > + | | finish > + | caret > + start > +@end smallexample > + > +Tokens coming out of libcpp have locations of the form ``caret == start'', > +such as for ``foo'' here: > + > +@smallexample > + a = foo && bar; > + ^~~ > + | | > + | finish > + caret == start > +@end smallexample > + > +Compound expressions should be reported using the location of the > +expression as a whole, rather than just of one token within it. > + > +For example, in @code{-Wformat}, rather than underlining just the first > +token of a bad argument: > + > +@smallexample > + printf("hello %i %s", (long)0, "world); > + ~^ ~ > + %li > +@end smallexample > + > +the whole of the expression should be underlined, so that the user can > +easily identify what is being referred to: > + > +@smallexample > + printf("hello %i %s", (long)0, "world); > + ~^ ~~~~~~~ > + %li > +@end smallexample > + > +@c this was r251239 > + > +Avoid using the @code{input_location} global, and the diagnostic functions > +that implicitly use it - use @code{error_at} and @code{warning_at} rather > +than @code{error} and @code{warning}. > + > +@c TODO labelling of ranges > + > +@subsection Coding Conventions > + > +See the @uref{https://gcc.gnu.org/codingconventions.html#Diagnostics, > +diagnostics section} of the GCC coding conventions. > + > +In the C++ frontend, when comparing two types in a message, use @code{%H} > +and @code{%I} rather tha @code{%T}, as this allows the diagnostics > +subsystem to highlight differences between template-based types. > + > +Use @code{auto_diagnostic_group} when issuing multiple related > +diagnostics (seen in various examples on this page). This informs the > +diagnostic subsystem that all diagnostics issued within the lifetime > +of the @code{auto_diagnostic_group} are related. (Currently it doesn't > +do anything with this information, but we may implement that in the > +future). > + > +@subsection Spelling and Terminology > + > +See the @uref{https://gcc.gnu.org/codingconventions.html#Spelling > +Spelling, terminology and markup} section of the GCC coding conventions. > + > +@subsection Tense of messages > +Syntax errors occur in the present tense e.g. > + > +@smallexample > +error_at (loc, "cannot convert %qH to %qI"); > +@end smallexample > + > +and thus > + > +@smallexample > +// CORRECT: usage of present tense: > +error: cannot convert 'int' to 'void *' > +@end smallexample > + > +rather than > + > +@smallexample > +// BAD: usage of past tense: > +error: could not convert 'int' to 'void *' > +@end smallexample > + > +Predictions about run-time behavior should be described in the future tense > e.g. > + > +@smallexample > +warning_at (loc, OPT_some_warning, > + "this code will read past the end of the array"); > +@end smallexample > + > +@subsection Fix-it hints > + > +GCC's diagnostic subsystem can emit ``fix-it hints'': small suggested > +edits to the user's source code. > + > +They are printed by default underneath the code in question. They > +can also be viewed via @option{-fdiagnostics-generate-patch} and > +@option{-fdiagnostics-parseable-fixits}. With the latter, an IDE > +ought to be able to offer to automatically apply the suggested fix. > + > +Fix-it hints can be added to a diagnostic by using a @code{rich_location} > +rather than a @code{location_t} - the fix-it hints are added to the > +@code{rich_location} using one of the various @code{add_fixit} member > +functions of @code{rich_location}. They are documented with > +@code{rich_location} in @file{libcpp/line-map.h}. > +It's easiest to use the @code{gcc_rich_location} subclass of > +@code{rich_location} found in @file{gcc-rich-location.h}. > + > +For example: > + > +@smallexample > + if (const char *suggestion = hint.suggestion ()) > + @{ > + gcc_rich_location richloc (location); > + richloc.add_fixit_replace (suggestion); > + error_at (&richloc, > + "%qE does not name a type; did you mean %qs?", > + id, suggestion); > + @} > +@end smallexample > + > +which can lead to: > + > +@smallexample > +spellcheck-typenames.C:73:1: error: 'singed' does not name a type; did you > mean 'signed'? > +73 | singed char ch; > + | ^~~~~~ > + | signed > +@end smallexample > + > +Non-trivial edits can be built up by adding multiple fix-it hints to one > +@code{rich_location}. It's best to express the edits in terms of the > +locations of individual tokens. Various handy functions for adding > +fix-it hints for idiomatic C and C++ can be seen in > +@file{gcc-rich-location.h}. > + > +@subsubsection Fix-it hints should be ``applyable'' > + > +When implementing a fix-it hint, please verify that the suggested edit > +leads to fixed, compilable code. (Unfortunately, this currently must be > +done by hand using @option{-fdiagnostics-generate-patch}. It would be > +good to have an automated way of verifying that fix-it hints actually fix > +the code). > + > +For example, a ``gotcha'' here is to forget to add a space when adding a > +missing reserved word. Consider a C++ fix-it hint that adds > +@code{typename} in front of a template declaration. A naive way to > +implement this might be: > + > +@smallexample > +gcc_rich_location richloc (loc); > +// BAD: insertion is missing a trailing space > +richloc.add_fixit_insert_before ("typename"); > +error_at (&richloc, "need %<typename%> before %<%T::%E%> because " > + "%qT is a dependent scope", > + parser->scope, id, parser->scope); > +@end smallexample > + > +When applied to the code, this might lead to: > + > +@smallexample > +T::type x; > +@end smallexample > + > +being ``corrected'' to: > + > +@smallexample > +typenameT::type x; > +@end smallexample > + > +In this case, the correct thing to do is to add a trailing space after > +@code{typename}: > + > +@smallexample > +gcc_rich_location richloc (loc); > +// OK: note that here we have a trailing space > +richloc.add_fixit_insert_before ("typename "); > +error_at (&richloc, "need %<typename%> before %<%T::%E%> because " > + "%qT is a dependent scope", > + parser->scope, id, parser->scope); > +@end smallexample > + > +leading to this corrected code: > + > +@smallexample > +typename T::type x; > +@end smallexample > + > +@subsubsection Express deletion in terms of deletion, not replacement > + > +It's best to express deletion suggestions in terms of deletion fix-it > +hints, rather than replacement fix-it hints. For example, consider this: > + > +@smallexample > + auto_diagnostic_group d; > + gcc_rich_location richloc (location_of (retval)); > + tree name = DECL_NAME (arg); > + richloc.add_fixit_replace (IDENTIFIER_POINTER (name)); > + warning_at (&richloc, OPT_Wredundant_move, > + "redundant move in return statement"); > +@end smallexample > + > +which is intended to e.g. replace a @code{std::move} with the underlying > +value: > + > +@smallexample > + return std::move (retval); > + ~~~~~~~~~~^~~~~~~~ > + retval > +@end smallexample > + > +where the change has been expressed as replacement, replacing > +with the name of the declaration. > + > +This works for simple cases, but consider this case: > + > +@smallexample > +#ifdef SOME_CONFIG_FLAG > +# define CONFIGURY_GLOBAL global_a > +#else > +# define CONFIGURY_GLOBAL global_b > +#endif > + > +int fn () > +@{ > + return std::move (CONFIGURY_GLOBAL /* some comment */); > +@} > +@end smallexample > + > +The above implementation will erroneously strip out the macro and the > +comment in the fix-it hint: > + > +@smallexample > + return std::move (CONFIGURY_GLOBAL /* some comment */); > + ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + global_a > +@end smallexample > + > +and thus this resulting code: > + > +@smallexample > + return global_a; > +@end smallexample > + > +It's better to do deletions in terms of deletions; deleting the > +@code{std::move (} and the trailing close-paren, leading to > +this: > + > +@smallexample > + return std::move (CONFIGURY_GLOBAL /* some comment */); > + ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + CONFIGURY_GLOBAL /* some comment */ > +@end smallexample > + > +and thus this result: > + > +@smallexample > + return CONFIGURY_GLOBAL /* some comment */; > +@end smallexample > + > +Unfortunately, the pertinent @code{location_t} values are not always > +available. > + > +@c the above was https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01474.html > + > +@subsubsection Multiple suggestions > + > +In the rare cases where you need to suggest more than one mutually > +exclusive solution to a problem, this can be done by emitting > +multiple notes and calling > +@code{rich_location::fixits_cannot_be_auto_applied} on each note's > +@code{rich_location}. If this is called, then the fix-it hints in > +the @code{rich_location} will be printed, but will not be added to > +generated patches. > + > + > +@node Guidelines for Options > +@cindex Options, Guidelines for > +@cindex Guidelines for Options > + > +@section Guidelines for Options > + > +@c TODO > -- > 1.8.5.3 > >
On 10/12/2018 09:43 AM, David Malcolm wrote: > Here's a proposed "User Experience Guidelines" section for our > internals manual > > It's a mixture of proposed policy, together with notes on how to > implement the recommendations. > > Thoughts? To improve consistency among diagnostic messages it's important to have a set of guidelines in place. Thank you for taking the first step! > > gcc/ChangeLog: > * Makefile.in (TEXI_GCCINT_FILES): Add ux.texi. > * doc/gccint.texi: Include ux.texi and use it in top-level menu. > * doc/ux.texi: New file. > --- > gcc/Makefile.in | 2 +- > gcc/doc/gccint.texi | 2 + > gcc/doc/ux.texi | 455 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 458 insertions(+), 1 deletion(-) > create mode 100644 gcc/doc/ux.texi > > diff --git a/gcc/Makefile.in b/gcc/Makefile.in > index 70efab7..3f05e95 100644 > --- a/gcc/Makefile.in > +++ b/gcc/Makefile.in > @@ -3176,7 +3176,7 @@ TEXI_GCCINT_FILES = gccint.texi gcc-common.texi gcc-vers.texi \ > gnu.texi gpl_v3.texi fdl.texi contrib.texi languages.texi \ > sourcebuild.texi gty.texi libgcc.texi cfg.texi tree-ssa.texi \ > loop.texi generic.texi gimple.texi plugins.texi optinfo.texi \ > - match-and-simplify.texi poly-int.texi > + match-and-simplify.texi ux.texi poly-int.texi > > TEXI_GCCINSTALL_FILES = install.texi install-old.texi fdl.texi \ > gcc-common.texi gcc-vers.texi > diff --git a/gcc/doc/gccint.texi b/gcc/doc/gccint.texi > index 1a1af41..2554b31 100644 > --- a/gcc/doc/gccint.texi > +++ b/gcc/doc/gccint.texi > @@ -125,6 +125,7 @@ Additional tutorial information is linked to from > * LTO:: Using Link-Time Optimization. > > * Match and Simplify:: How to write expression simplification patterns for GIMPLE and GENERIC > +* User Experience Guidelines:: Guidelines for implementing diagnostics and options. > * Funding:: How to help assure funding for free software. > * GNU Project:: The GNU Project and GNU/Linux. > > @@ -162,6 +163,7 @@ Additional tutorial information is linked to from > @include plugins.texi > @include lto.texi > @include match-and-simplify.texi > +@include ux.texi > > @include funding.texi > @include gnu.texi > diff --git a/gcc/doc/ux.texi b/gcc/doc/ux.texi > new file mode 100644 > index 0000000..87ff599 > --- /dev/null > +++ b/gcc/doc/ux.texi > @@ -0,0 +1,455 @@ > +@c Copyright (C) 2018 Free Software Foundation, Inc. > +@c Free Software Foundation, Inc. > +@c This is part of the GCC manual. > +@c For copying conditions, see the file gcc.texi. > + > +@node User Experience Guidelines > +@chapter User Experience Guidelines > +@cindex User Experience Guidelines > +@cindex Guidelines, User Experience > + > +To borrow a slogan from > + @uref{https://elm-lang.org/blog/compilers-as-assistants, Elm}, > + > +@quotation > +@strong{Compilers should be assistants, not adversaries.} A compiler should > +not just detect bugs, it should then help you understand why there is a bug. > +It should not berate you in a robot voice, it should give you specific hints > +that help you write better code. Ultimately, a compiler should make > +programming faster and more fun! > +@author Evan Czaplicki > +@end quotation > + > +This chapter provides guidelines on how to implement diagnostics and > +command-line options in ways that we hope achieve the above ideal. > + > +@menu > +* Guidelines for Diagnostics:: How to implement diagnostics. > +* Guidelines for Options:: Guidelines for command-line options. > +@end menu > + > + > +@node Guidelines for Diagnostics > +@cindex Guidelines for Diagnostics > +@cindex Diagnostics, Guidelines for > + > +@section Guidelines for Diagnostics > + > +@subsection Talk in terms of the user's code > + > +Diagnostics should be worded in terms of the user's source code, and the > +source language, rather than GCC's own implementation details. > + > +@subsection Diagnostics are ``actionable'' > + > +A good diagnostic is @emph{actionable}: it should assist the user in > +taking action. > + > +Consider what an end-user will want to do when encountering a diagnostic. > + > +Given an error, an end-user will think: ``How do I fix this?'' > + > +Given a warning, an end-user will want to review the warning and think: > + > +@itemize @bullet > +@item > +``Is this a ``true'' result?'' Sounds like you mean a true positive here but I'm not 100% sure so expanding on this a bit it might be helpful -- maybe simply phrasing it as "is this a real problem?" > +@item > +``Do I care?'' > +@item > +if they decide it's genuine: ``How do I fix this?'' > +@end itemize > + > +A good diagnostic provides pertinent information to allow the user to > +easily answer the above questions. > + > +@subsection The user's attention is important > + > +A perfect compiler would issue a warning on every aspect of the user's > +source code that ought to be fixed, and issue no other warnings. > +Naturally, this ideal is impossible to achieve. > + > +Warnings should have a good ``signal:noise ratio'': we should have few > +``false positives'' (falsely issuing a warning when no warning is > +warranted) and few ``false negatives'' (failing to issue a warning when > +one @emph{is} justified). > + > +Note that a ``false positive'' can mean, in practice, a warning that the > +user doesn't agree with. Ideally a diagnostic should contain enough > +information to allow the user to make an informed choice about whether > +they should care (and how to fix it), but a balance must be drawn against > +overloading the user with irrelevant data. > + > +It's worth testing a new warning on many instances of real-world code, > +written by different people, and seeing what it complains about, and > +what it doesn't complain about. This may suggest heuristics that > +silence common false positives. > + > +@subsection Make mismatches clear > + > +Many diagnostics relate to a mismatch between two different places in the > +user's source code. Examples include: > +@itemize @bullet > + @item > + a type mismatch, where the type at a usage site does not match the type > + at a declaration > + > + @item > + the argument count at a call site does not match the parameter count > + at the declaration > + > + @item > + something is erroneously duplicated (e.g. an error, due to breaking a > + uniqueness requirement, or a warning, if it's suggestive of a bug) > + > + @item > + an ``opened'' syntactic construct (such as an open-parenthesis) is not > + closed > + > + @c TODO: more examples? The area of attributes and attribute checking is rife with examples of things to improve. A poster child for a warning with an unclear rationale is: 'foobar' attribute ignored. (There are nearly a hundred of these.) Most are due to a mismatch between the kind of a symbol the attribute is expected to apply to (function, type, variable, etc.) and the symbol it is being used with. This kind of a mismatch should also be made clear. Doing it right could be a little tedious because mentioning both kinds of symbols involved could lead to a combinatorial explosion of messages due to internationalization. A clever use of notes would help. (Improving the design of struct attribute_spec and handling this in the middle-end would relieve each attribute handler from having to jump through these hoops.) > +@end itemize > + > +In each case, the diagnostic should indicate @strong{both} pertinent > +locations (so that the user can easily see the problem and how to fix it). > + > +The standard way to do this is with a note (via @code{inform}). For > +example: > + > +@smallexample > + auto_diagnostic_group d; > + if (warning_at (loc, OPT_Wduplicated_cond, > + "duplicated %<if%> condition")) > + inform (EXPR_LOCATION (t), "previously used here"); > +@end smallexample I wonder if this could also be used as an example of a missing detail: is the relationship necessarily obvious between the warning and the note? (Can there be two warnings with a note on the same line? If not here, I know there can be in other warnings.) If not, is it worth solving in a consistent way? > + > +For cases involving punctuation where the locations might be near > +each other, they can be conditionally consolidated via > +@code{gcc_rich_location::add_location_if_nearby}: > + > +@smallexample > + auto_diagnostic_group d; > + gcc_rich_location richloc (primary_loc); > + bool added secondary = richloc.add_location_if_nearby (secondary_loc); > + error_at (&richloc, "main message"); > + if (!added secondary) > + inform (secondary_loc, "message for secondary"); > +@end smallexample > + > +This will emit either one diagnostic with two locations: > +@smallexample > + demo.c:42:10: error: main message > + (foo) > + ~ ^ > +@end smallexample > + > +or two diagnostics: > + > +@smallexample > + demo.c:42:4: error: main message > + foo) > + ^ > + demo.c:40:2: note: message for secondary > + ( > + ^ > +@end smallexample > + > +@subsection Location Information > + > +GCC's @code{location_t} type can support both ``ordinary'' locations, > +and locations relating to a macro expansion. > + > +As of GCC 6, ordinary locations changed from supporting just a > +point in the user's source code to supporting three points: the > +``caret'' location, plus a start and a finish: > + > +@smallexample > + a = foo && bar; > + ~~~~^~~~~~ > + | | | > + | | finish > + | caret > + start > +@end smallexample > + > +Tokens coming out of libcpp have locations of the form ``caret == start'', > +such as for ``foo'' here: > + > +@smallexample > + a = foo && bar; > + ^~~ > + | | > + | finish > + caret == start > +@end smallexample > + > +Compound expressions should be reported using the location of the > +expression as a whole, rather than just of one token within it. > + > +For example, in @code{-Wformat}, rather than underlining just the first > +token of a bad argument: > + > +@smallexample > + printf("hello %i %s", (long)0, "world); > + ~^ ~ > + %li > +@end smallexample > + > +the whole of the expression should be underlined, so that the user can > +easily identify what is being referred to: > + > +@smallexample > + printf("hello %i %s", (long)0, "world); > + ~^ ~~~~~~~ > + %li > +@end smallexample > + > +@c this was r251239 This is really nice. My experience from the sprintf warnings is that it was quite to get to work the way I wanted to (put the caret and underlining just in the right place). I'm not sure who else has worked with this and what their experience was but I wonder if a separate tutorial showing how to make this work would help people get more comfortable with it. > + > +Avoid using the @code{input_location} global, and the diagnostic functions > +that implicitly use it - use @code{error_at} and @code{warning_at} rather > +than @code{error} and @code{warning}. > + > +@c TODO labelling of ranges > + > +@subsection Coding Conventions > + > +See the @uref{https://gcc.gnu.org/codingconventions.html#Diagnostics, > +diagnostics section} of the GCC coding conventions. > + > +In the C++ frontend, when comparing two types in a message, use @code{%H} > +and @code{%I} rather tha @code{%T}, as this allows the diagnostics > +subsystem to highlight differences between template-based types. > + > +Use @code{auto_diagnostic_group} when issuing multiple related > +diagnostics (seen in various examples on this page). This informs the > +diagnostic subsystem that all diagnostics issued within the lifetime > +of the @code{auto_diagnostic_group} are related. (Currently it doesn't > +do anything with this information, but we may implement that in the > +future). Same here. I have never used this and even though I saw the %H and %I patches go in I probably wouldn't have thought of using these codes. Having a little tutorial (or an example showing the effect of these directives) might help. > + > +@subsection Spelling and Terminology > + > +See the @uref{https://gcc.gnu.org/codingconventions.html#Spelling > +Spelling, terminology and markup} section of the GCC coding conventions. > + > +@subsection Tense of messages > +Syntax errors occur in the present tense e.g. > + > +@smallexample > +error_at (loc, "cannot convert %qH to %qI"); > +@end smallexample > + > +and thus > + > +@smallexample > +// CORRECT: usage of present tense: > +error: cannot convert 'int' to 'void *' > +@end smallexample This sounds fine. It would be nice to change the few instances of the past tense. I think past tense is appropriate in runtime messages like "could not open file" so it might be worth mentioning. > + > +rather than > + > +@smallexample > +// BAD: usage of past tense: > +error: could not convert 'int' to 'void *' > +@end smallexample > + > +Predictions about run-time behavior should be described in the future tense e.g. > + > +@smallexample > +warning_at (loc, OPT_some_warning, > + "this code will read past the end of the array"); > +@end smallexample I don't think this will always achieve a good result. Consider warnings like -Wuninitialized: 'x' is used uninitialized in this function A slightly different example is -Walloca, -Warray-bounds, and -Wvla: it makes more sense (to me) to say "value is too large" or "index is out of bounds" than "it will be too large" etc. It would be fine for warnings like -Wformat-truncation but all it will do there is add a word without making things any clearer: "output truncated" vs "output will be truncated." In general, it's also not always the case that the code will necessarily be executed. It could be eliminated or replaced by a trap (as we discussed at Cauldron), so saying "this will happen" when we know something else entirely might happen (and what exactly that is might depend on options) would be misleading. > + > +@subsection Fix-it hints There's a wealth of information in this section and it reads like a tutorial as much as, if not more than guidelines. Would separating the two make sense? I.e., have a standalone tutorial page and just the most important high-level guidelines here? Besides the topics you already covered, some others that I think would be useful to cover are those I mentioned at Cauldron: e.g., how to choose where to implement a warning (pattern rules vs flow-based rules), the common gotchas to look out for there, etc. I'll be happy to contribute those once the first version is agreed on and committed. Martin > + > +GCC's diagnostic subsystem can emit ``fix-it hints'': small suggested > +edits to the user's source code. > + > +They are printed by default underneath the code in question. They > +can also be viewed via @option{-fdiagnostics-generate-patch} and > +@option{-fdiagnostics-parseable-fixits}. With the latter, an IDE > +ought to be able to offer to automatically apply the suggested fix. > + > +Fix-it hints can be added to a diagnostic by using a @code{rich_location} > +rather than a @code{location_t} - the fix-it hints are added to the > +@code{rich_location} using one of the various @code{add_fixit} member > +functions of @code{rich_location}. They are documented with > +@code{rich_location} in @file{libcpp/line-map.h}. > +It's easiest to use the @code{gcc_rich_location} subclass of > +@code{rich_location} found in @file{gcc-rich-location.h}. > + > +For example: > + > +@smallexample > + if (const char *suggestion = hint.suggestion ()) > + @{ > + gcc_rich_location richloc (location); > + richloc.add_fixit_replace (suggestion); > + error_at (&richloc, > + "%qE does not name a type; did you mean %qs?", > + id, suggestion); > + @} > +@end smallexample > + > +which can lead to: > + > +@smallexample > +spellcheck-typenames.C:73:1: error: 'singed' does not name a type; did you mean 'signed'? > +73 | singed char ch; > + | ^~~~~~ > + | signed > +@end smallexample > + > +Non-trivial edits can be built up by adding multiple fix-it hints to one > +@code{rich_location}. It's best to express the edits in terms of the > +locations of individual tokens. Various handy functions for adding > +fix-it hints for idiomatic C and C++ can be seen in > +@file{gcc-rich-location.h}. > + > +@subsubsection Fix-it hints should be ``applyable'' > + > +When implementing a fix-it hint, please verify that the suggested edit > +leads to fixed, compilable code. (Unfortunately, this currently must be > +done by hand using @option{-fdiagnostics-generate-patch}. It would be > +good to have an automated way of verifying that fix-it hints actually fix > +the code). > + > +For example, a ``gotcha'' here is to forget to add a space when adding a > +missing reserved word. Consider a C++ fix-it hint that adds > +@code{typename} in front of a template declaration. A naive way to > +implement this might be: > + > +@smallexample > +gcc_rich_location richloc (loc); > +// BAD: insertion is missing a trailing space > +richloc.add_fixit_insert_before ("typename"); > +error_at (&richloc, "need %<typename%> before %<%T::%E%> because " > + "%qT is a dependent scope", > + parser->scope, id, parser->scope); > +@end smallexample > + > +When applied to the code, this might lead to: > + > +@smallexample > +T::type x; > +@end smallexample > + > +being ``corrected'' to: > + > +@smallexample > +typenameT::type x; > +@end smallexample > + > +In this case, the correct thing to do is to add a trailing space after > +@code{typename}: > + > +@smallexample > +gcc_rich_location richloc (loc); > +// OK: note that here we have a trailing space > +richloc.add_fixit_insert_before ("typename "); > +error_at (&richloc, "need %<typename%> before %<%T::%E%> because " > + "%qT is a dependent scope", > + parser->scope, id, parser->scope); > +@end smallexample > + > +leading to this corrected code: > + > +@smallexample > +typename T::type x; > +@end smallexample > + > +@subsubsection Express deletion in terms of deletion, not replacement > + > +It's best to express deletion suggestions in terms of deletion fix-it > +hints, rather than replacement fix-it hints. For example, consider this: > + > +@smallexample > + auto_diagnostic_group d; > + gcc_rich_location richloc (location_of (retval)); > + tree name = DECL_NAME (arg); > + richloc.add_fixit_replace (IDENTIFIER_POINTER (name)); > + warning_at (&richloc, OPT_Wredundant_move, > + "redundant move in return statement"); > +@end smallexample > + > +which is intended to e.g. replace a @code{std::move} with the underlying > +value: > + > +@smallexample > + return std::move (retval); > + ~~~~~~~~~~^~~~~~~~ > + retval > +@end smallexample > + > +where the change has been expressed as replacement, replacing > +with the name of the declaration. > + > +This works for simple cases, but consider this case: > + > +@smallexample > +#ifdef SOME_CONFIG_FLAG > +# define CONFIGURY_GLOBAL global_a > +#else > +# define CONFIGURY_GLOBAL global_b > +#endif > + > +int fn () > +@{ > + return std::move (CONFIGURY_GLOBAL /* some comment */); > +@} > +@end smallexample > + > +The above implementation will erroneously strip out the macro and the > +comment in the fix-it hint: > + > +@smallexample > + return std::move (CONFIGURY_GLOBAL /* some comment */); > + ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + global_a > +@end smallexample > + > +and thus this resulting code: > + > +@smallexample > + return global_a; > +@end smallexample > + > +It's better to do deletions in terms of deletions; deleting the > +@code{std::move (} and the trailing close-paren, leading to > +this: > + > +@smallexample > + return std::move (CONFIGURY_GLOBAL /* some comment */); > + ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + CONFIGURY_GLOBAL /* some comment */ > +@end smallexample > + > +and thus this result: > + > +@smallexample > + return CONFIGURY_GLOBAL /* some comment */; > +@end smallexample > + > +Unfortunately, the pertinent @code{location_t} values are not always > +available. > + > +@c the above was https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01474.html > + > +@subsubsection Multiple suggestions > + > +In the rare cases where you need to suggest more than one mutually > +exclusive solution to a problem, this can be done by emitting > +multiple notes and calling > +@code{rich_location::fixits_cannot_be_auto_applied} on each note's > +@code{rich_location}. If this is called, then the fix-it hints in > +the @code{rich_location} will be printed, but will not be added to > +generated patches. > + > + > +@node Guidelines for Options > +@cindex Options, Guidelines for > +@cindex Guidelines for Options > + > +@section Guidelines for Options > + > +@c TODO >
Another thanks for doing this. Martin Sebor <msebor@gmail.com> writes: > On 10/12/2018 09:43 AM, David Malcolm wrote: >> +Avoid using the @code{input_location} global, and the diagnostic functions >> +that implicitly use it - use @code{error_at} and @code{warning_at} rather >> +than @code{error} and @code{warning}. >> + >> +@c TODO labelling of ranges >> + >> +@subsection Coding Conventions >> + >> +See the @uref{https://gcc.gnu.org/codingconventions.html#Diagnostics, >> +diagnostics section} of the GCC coding conventions. >> + >> +In the C++ frontend, when comparing two types in a message, use @code{%H} >> +and @code{%I} rather tha @code{%T}, as this allows the diagnostics >> +subsystem to highlight differences between template-based types. >> + >> +Use @code{auto_diagnostic_group} when issuing multiple related >> +diagnostics (seen in various examples on this page). This informs the >> +diagnostic subsystem that all diagnostics issued within the lifetime >> +of the @code{auto_diagnostic_group} are related. (Currently it doesn't >> +do anything with this information, but we may implement that in the >> +future). > > Same here. I have never used this and even though I saw the %H > and %I patches go in I probably wouldn't have thought of using > these codes. Having a little tutorial (or an example showing > the effect of these directives) might help. Yeah, agree a bad %T vs. good %H/%I example would help here. >> + >> +@subsection Spelling and Terminology >> + >> +See the @uref{https://gcc.gnu.org/codingconventions.html#Spelling >> +Spelling, terminology and markup} section of the GCC coding conventions. >> + >> +@subsection Tense of messages >> +Syntax errors occur in the present tense e.g. >> + >> +@smallexample >> +error_at (loc, "cannot convert %qH to %qI"); >> +@end smallexample >> + >> +and thus >> + >> +@smallexample >> +// CORRECT: usage of present tense: >> +error: cannot convert 'int' to 'void *' >> +@end smallexample > > This sounds fine. It would be nice to change the few instances > of the past tense. I think past tense is appropriate in runtime > messages like "could not open file" so it might be worth mentioning. Yeah. Is the conversion example above really a case of fixing the tense, or of fixing the error to be in terms of the user's code? The implicit subject in "could not convert ..." is clearly "I"/"the compiler", making the past tense appropriate. If the compiler failed to find something, it wouldn't make sense to do a straight tense swap from "failed to find..." to "fail to find...". In "cannot convert ..." I think the subject becomes "you"/"the code" and the message becomes a prohibition. That's a good thing because like you say the error should be in terms of the user's code (although I guess, going back to your quote at the start of the page, that also means it has a slightly more lecturing tone -- "hey, you can't do that!") I guess using the present tense also means that we shouldn't treat the code as a narrative, so maybe the examples should include diagnostics that correctly talk in terms of the user's code but use the wrong tense. E.g. if we don't want to use the past tense: error ("redefinition of %q#D", value); inform (DECL_SOURCE_LOCATION (value), "%q#D previously defined here", value); should presumably be something like: error ("redefinition of %q#D", value); inform (DECL_SOURCE_LOCATION (value), "previous definition of %q#D is here", value); (Although TBH the original or: "previous definition of %q#D was here" sound more natural to me.) >> +rather than >> + >> +@smallexample >> +// BAD: usage of past tense: >> +error: could not convert 'int' to 'void *' >> +@end smallexample >> + >> +Predictions about run-time behavior should be described in the future tense e.g. >> + >> +@smallexample >> +warning_at (loc, OPT_some_warning, >> + "this code will read past the end of the array"); >> +@end smallexample > > I don't think this will always achieve a good result. Consider > warnings like -Wuninitialized: > > 'x' is used uninitialized in this function > > A slightly different example is -Walloca, -Warray-bounds, and > -Wvla: it makes more sense (to me) to say "value is too large" > or "index is out of bounds" than "it will be too large" etc. > > It would be fine for warnings like -Wformat-truncation but all > it will do there is add a word without making things any clearer: > "output truncated" vs "output will be truncated." FWIW, this would also be the opposite of the GCC convention for documentation, where predictions about compiler behaviour in particular situations should be given in the present tense rather than the future tense. Thanks, Richard
On Sun, 2018-10-14 at 11:01 -0600, Martin Sebor wrote: > On 10/12/2018 09:43 AM, David Malcolm wrote: > > Here's a proposed "User Experience Guidelines" section for our > > internals manual > > > > It's a mixture of proposed policy, together with notes on how to > > implement the recommendations. > > > > Thoughts? > > To improve consistency among diagnostic messages it's important > to have a set of guidelines in place. Thank you for taking > the first step! I'm not as interested in "consistency" here as I am in what I'm calling "actionable" diagnostics ("actionableness"?). For a reductio ad absurdum, consider if we replaced all of our messages with the empty string, so that we merely issued "error" or "warning" to the user. That would achieve "consistency" for all diagnostic messages, but wouldn't be helpful to the user. "Being an assistant to the user rather than an adversary" is the high- level goal I want to emphasize. Consistency is good, and will usually be in service to the primary goal (as it helps the user learn the program and be able to figure things out from past experience), but if we can help the user by special-casing certain things, that can take precedence over consistency of output. Various comments inline below. > > > > gcc/ChangeLog: > > * Makefile.in (TEXI_GCCINT_FILES): Add ux.texi. > > * doc/gccint.texi: Include ux.texi and use it in top-level > > menu. > > * doc/ux.texi: New file. > > --- > > gcc/Makefile.in | 2 +- > > gcc/doc/gccint.texi | 2 + > > gcc/doc/ux.texi | 455 > > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 458 insertions(+), 1 deletion(-) > > create mode 100644 gcc/doc/ux.texi > > > > diff --git a/gcc/Makefile.in b/gcc/Makefile.in > > index 70efab7..3f05e95 100644 > > --- a/gcc/Makefile.in > > +++ b/gcc/Makefile.in > > @@ -3176,7 +3176,7 @@ TEXI_GCCINT_FILES = gccint.texi gcc- > > common.texi gcc-vers.texi \ > > gnu.texi gpl_v3.texi fdl.texi contrib.texi languages.texi > > \ > > sourcebuild.texi gty.texi libgcc.texi cfg.texi tree- > > ssa.texi \ > > loop.texi generic.texi gimple.texi plugins.texi > > optinfo.texi \ > > - match-and-simplify.texi poly-int.texi > > + match-and-simplify.texi ux.texi poly-int.texi > > > > TEXI_GCCINSTALL_FILES = install.texi install-old.texi fdl.texi > > \ > > gcc-common.texi gcc-vers.texi > > diff --git a/gcc/doc/gccint.texi b/gcc/doc/gccint.texi > > index 1a1af41..2554b31 100644 > > --- a/gcc/doc/gccint.texi > > +++ b/gcc/doc/gccint.texi > > @@ -125,6 +125,7 @@ Additional tutorial information is linked to > > from > > * LTO:: Using Link-Time Optimization. > > > > * Match and Simplify:: How to write expression simplification > > patterns for GIMPLE and GENERIC > > +* User Experience Guidelines:: Guidelines for implementing > > diagnostics and options. > > * Funding:: How to help assure funding for free software. > > * GNU Project:: The GNU Project and GNU/Linux. > > > > @@ -162,6 +163,7 @@ Additional tutorial information is linked to > > from > > @include plugins.texi > > @include lto.texi > > @include match-and-simplify.texi > > +@include ux.texi > > > > @include funding.texi > > @include gnu.texi > > diff --git a/gcc/doc/ux.texi b/gcc/doc/ux.texi > > new file mode 100644 > > index 0000000..87ff599 > > --- /dev/null > > +++ b/gcc/doc/ux.texi > > @@ -0,0 +1,455 @@ > > +@c Copyright (C) 2018 Free Software Foundation, Inc. > > +@c Free Software Foundation, Inc. > > +@c This is part of the GCC manual. > > +@c For copying conditions, see the file gcc.texi. > > + > > +@node User Experience Guidelines > > +@chapter User Experience Guidelines > > +@cindex User Experience Guidelines > > +@cindex Guidelines, User Experience > > + > > +To borrow a slogan from > > + @uref{https://elm-lang.org/blog/compilers-as-assistants, Elm}, > > + > > +@quotation > > +@strong{Compilers should be assistants, not adversaries.} A > > compiler should > > +not just detect bugs, it should then help you understand why there > > is a bug. > > +It should not berate you in a robot voice, it should give you > > specific hints > > +that help you write better code. Ultimately, a compiler should > > make > > +programming faster and more fun! > > +@author Evan Czaplicki > > +@end quotation > > + > > +This chapter provides guidelines on how to implement diagnostics > > and > > +command-line options in ways that we hope achieve the above ideal. > > + > > +@menu > > +* Guidelines for Diagnostics:: How to implement diagnostics. > > +* Guidelines for Options:: Guidelines for command-line > > options. > > +@end menu > > + > > + > > +@node Guidelines for Diagnostics > > +@cindex Guidelines for Diagnostics > > +@cindex Diagnostics, Guidelines for > > + > > +@section Guidelines for Diagnostics > > + > > +@subsection Talk in terms of the user's code > > + > > +Diagnostics should be worded in terms of the user's source code, > > and the > > +source language, rather than GCC's own implementation details. > > + > > +@subsection Diagnostics are ``actionable'' > > + > > +A good diagnostic is @emph{actionable}: it should assist the user > > in > > +taking action. > > + > > +Consider what an end-user will want to do when encountering a > > diagnostic. > > + > > +Given an error, an end-user will think: ``How do I fix this?'' > > + > > +Given a warning, an end-user will want to review the warning and > > think: > > + > > +@itemize @bullet > > +@item > > +``Is this a ``true'' result?'' > > Sounds like you mean a true positive here but I'm not 100% sure > so expanding on this a bit it might be helpful -- maybe simply > phrasing it as "is this a real problem?" Thanks. Yes, I've updated it to read "is this a real problem?" > > +@item > > +``Do I care?'' > > +@item > > +if they decide it's genuine: ``How do I fix this?'' > > +@end itemize > > + > > +A good diagnostic provides pertinent information to allow the user > > to > > +easily answer the above questions. > > + > > +@subsection The user's attention is important > > + > > +A perfect compiler would issue a warning on every aspect of the > > user's > > +source code that ought to be fixed, and issue no other warnings. > > +Naturally, this ideal is impossible to achieve. > > + > > +Warnings should have a good ``signal:noise ratio'': we should have > > few > > +``false positives'' (falsely issuing a warning when no warning is > > +warranted) and few ``false negatives'' (failing to issue a warning > > when > > +one @emph{is} justified). > > + > > +Note that a ``false positive'' can mean, in practice, a warning > > that the > > +user doesn't agree with. Ideally a diagnostic should contain > > enough > > +information to allow the user to make an informed choice about > > whether > > +they should care (and how to fix it), but a balance must be drawn > > against > > +overloading the user with irrelevant data. > > + > > +It's worth testing a new warning on many instances of real-world > > code, > > +written by different people, and seeing what it complains about, > > and > > +what it doesn't complain about. This may suggest heuristics that > > +silence common false positives. > > + > > +@subsection Make mismatches clear > > + > > +Many diagnostics relate to a mismatch between two different places > > in the > > +user's source code. Examples include: > > +@itemize @bullet > > + @item > > + a type mismatch, where the type at a usage site does not match > > the type > > + at a declaration > > + > > + @item > > + the argument count at a call site does not match the parameter > > count > > + at the declaration > > + > > + @item > > + something is erroneously duplicated (e.g. an error, due to > > breaking a > > + uniqueness requirement, or a warning, if it's suggestive of a > > bug) > > + > > + @item > > + an ``opened'' syntactic construct (such as an open-parenthesis) > > is not > > + closed > > + > > + @c TODO: more examples? > > The area of attributes and attribute checking is rife with examples > of things to improve. A poster child for a warning with an unclear > rationale is: > > 'foobar' attribute ignored. > > (There are nearly a hundred of these.) Most are due to a mismatch > between the kind of a symbol the attribute is expected to apply to > (function, type, variable, etc.) and the symbol it is being used > with. This kind of a mismatch should also be made clear. Doing > it right could be a little tedious because mentioning both kinds > of symbols involved could lead to a combinatorial explosion of > messages due to internationalization. A clever use of notes > would help. (Improving the design of struct attribute_spec > and handling this in the middle-end would relieve each attribute > handler from having to jump through these hoops.) So should this be something like: warning: attribute 'foobar' on function 'quux' was ignored note: attribute 'foobar' is only applicable to types or somesuch. Maybe the UX guidelines should have something like: @subsection Precision of Wording Provide the user with details that allow them to identify what the problem is. For example, the vaguely-worded message: @smallexample warning: 'foobar' attribute ignored. @end smallexample doesn't tell the user why the attribute was ignored, or what kind of entity the compiler thought the attribute was being applied to. A better message would be: @smallexample warning: attribute 'foobar'on function 'quux' was ignored note: attribute 'foobar' is only applicable to types @end smallexample which spells out the missing information. It's sometimes useful to use a note to avoid a combinatorial explosion of possible messages. ...though we should have a more concrete example here. > > +@end itemize > > + > > +In each case, the diagnostic should indicate @strong{both} > > pertinent > > +locations (so that the user can easily see the problem and how to > > fix it). > > + > > +The standard way to do this is with a note (via > > @code{inform}). For > > +example: > > + > > +@smallexample > > + auto_diagnostic_group d; > > + if (warning_at (loc, OPT_Wduplicated_cond, > > + "duplicated %<if%> condition")) > > + inform (EXPR_LOCATION (t), "previously used here"); > > +@end smallexample > > I wonder if this could also be used as an example of a missing > detail: is the relationship necessarily obvious between the warning > and the note? (Can there be two warnings with a note on the same > line? If not here, I know there can be in other warnings.) If not, > is it worth solving in a consistent way? I think the above example is clearer given an example of output: demo.c: In function ‘test’: demo.c:5:17: warning: duplicated ‘if’ condition [-Wduplicated-cond] 5 | else if (flag > 3) | ~~~~~^~~ demo.c:3:12: note: previously used here 3 | if (flag > 3) | ~~~~~^~~ which I've added to my working copy. I'm not sure I follow your point about "two warnings with a note on the same line". Yes, there can be, but you'd get something like: warning: msg 1 note: msg 2 warning: msg 3 note: msg 4 Hopefully the wording is clear. -Wmisleading_indentation uses ellipses for the warning/note pairs, so it looks like: warning: msg 1... note: ...msg 2 warning: msg 3... note: ...msg 4 ...but that one is worded as one sentence, with a break at the ellipses. I wondered if we recommend that approach: use one sentence, with a trailing ellipsis on the warning, and a leading ellipsis on the note - but I don't think so. > > + > > +For cases involving punctuation where the locations might be near > > +each other, they can be conditionally consolidated via > > +@code{gcc_rich_location::add_location_if_nearby}: > > + > > +@smallexample > > + auto_diagnostic_group d; > > + gcc_rich_location richloc (primary_loc); > > + bool added secondary = richloc.add_location_if_nearby > > (secondary_loc); > > + error_at (&richloc, "main message"); > > + if (!added secondary) > > + inform (secondary_loc, "message for secondary"); > > +@end smallexample > > + > > +This will emit either one diagnostic with two locations: > > +@smallexample > > + demo.c:42:10: error: main message > > + (foo) > > + ~ ^ > > +@end smallexample > > + > > +or two diagnostics: > > + > > +@smallexample > > + demo.c:42:4: error: main message > > + foo) > > + ^ > > + demo.c:40:2: note: message for secondary > > + ( > > + ^ > > +@end smallexample > > + > > +@subsection Location Information > > + > > +GCC's @code{location_t} type can support both ``ordinary'' > > locations, > > +and locations relating to a macro expansion. > > + > > +As of GCC 6, ordinary locations changed from supporting just a > > +point in the user's source code to supporting three points: the > > +``caret'' location, plus a start and a finish: > > + > > +@smallexample > > + a = foo && bar; > > + ~~~~^~~~~~ > > + | | | > > + | | finish > > + | caret > > + start > > +@end smallexample > > + > > +Tokens coming out of libcpp have locations of the form ``caret == > > start'', > > +such as for ``foo'' here: > > + > > +@smallexample > > + a = foo && bar; > > + ^~~ > > + | | > > + | finish > > + caret == start > > +@end smallexample > > + > > +Compound expressions should be reported using the location of the > > +expression as a whole, rather than just of one token within it. > > + > > +For example, in @code{-Wformat}, rather than underlining just the > > first > > +token of a bad argument: > > + > > +@smallexample > > + printf("hello %i %s", (long)0, "world); > > + ~^ ~ > > + %li > > +@end smallexample > > + > > +the whole of the expression should be underlined, so that the user > > can > > +easily identify what is being referred to: > > + > > +@smallexample > > + printf("hello %i %s", (long)0, "world); > > + ~^ ~~~~~~~ > > + %li > > +@end smallexample > > + > > +@c this was r251239 > > This is really nice. My experience from the sprintf warnings is > that it was quite to get to work the way I wanted to (put the caret > and underlining just in the right place). I'm not sure who else > has worked with this and what their experience was but I wonder > if a separate tutorial showing how to make this work would help > people get more comfortable with it. The substring location stuff? IIRC this has gained a big comment at the top of the file since I subjected you to it, so hopefully it's easier now. How about something like: @subsection Format-string diagnostics Diagnostics that identify problems within format strings should identify the precise location within the format string of the problem, rather than just the location of the string as a whole. See the notes in @file{substring-locations.h} for implementation details. > > + > > +Avoid using the @code{input_location} global, and the diagnostic > > functions > > +that implicitly use it - use @code{error_at} and @code{warning_at} > > rather > > +than @code{error} and @code{warning}. > > + > > +@c TODO labelling of ranges > > + > > +@subsection Coding Conventions > > + > > +See the @uref{https://gcc.gnu.org/codingconventions.html#Diagnosti > > cs, > > +diagnostics section} of the GCC coding conventions. > > + > > +In the C++ frontend, when comparing two types in a message, use > > @code{%H} > > +and @code{%I} rather tha @code{%T}, as this allows the diagnostics > > +subsystem to highlight differences between template-based types. > > + > > +Use @code{auto_diagnostic_group} when issuing multiple related > > +diagnostics (seen in various examples on this page). This informs > > the > > +diagnostic subsystem that all diagnostics issued within the > > lifetime > > +of the @code{auto_diagnostic_group} are related. (Currently it > > doesn't > > +do anything with this information, but we may implement that in > > the > > +future). > > Same here. I have never used this and even though I saw the %H > and %I patches go in I probably wouldn't have thought of using > these codes. Having a little tutorial (or an example showing > the effect of these directives) might help. I'll try to add an example to every recommendation. > > + > > +@subsection Spelling and Terminology > > + > > +See the @uref{https://gcc.gnu.org/codingconventions.html#Spelling > > +Spelling, terminology and markup} section of the GCC coding > > conventions. > > + > > +@subsection Tense of messages > > +Syntax errors occur in the present tense e.g. > > + > > +@smallexample > > +error_at (loc, "cannot convert %qH to %qI"); > > +@end smallexample > > + > > +and thus > > + > > +@smallexample > > +// CORRECT: usage of present tense: > > +error: cannot convert 'int' to 'void *' > > +@end smallexample > > This sounds fine. It would be nice to change the few instances > of the past tense. I think past tense is appropriate in runtime > messages like "could not open file" so it might be worth mentioning. I might leave out the "Tense of messages" subsection from the patch for next time, since it seems like it will need more discussion > > + > > +rather than > > + > > +@smallexample > > +// BAD: usage of past tense: > > +error: could not convert 'int' to 'void *' > > +@end smallexample > > + > > +Predictions about run-time behavior should be described in the > > future tense e.g. > > + > > +@smallexample > > +warning_at (loc, OPT_some_warning, > > + "this code will read past the end of the array"); > > +@end smallexample > > I don't think this will always achieve a good result. Consider > warnings like -Wuninitialized: > > 'x' is used uninitialized in this function > > A slightly different example is -Walloca, -Warray-bounds, and > -Wvla: it makes more sense (to me) to say "value is too large" > or "index is out of bounds" than "it will be too large" etc. > > It would be fine for warnings like -Wformat-truncation but all > it will do there is add a word without making things any clearer: > "output truncated" vs "output will be truncated." > > In general, it's also not always the case that the code will > necessarily be executed. It could be eliminated or replaced > by a trap (as we discussed at Cauldron), so saying "this will > happen" when we know something else entirely might happen > (and what exactly that is might depend on options) would be > misleading. > > > + > > +@subsection Fix-it hints > > There's a wealth of information in this section and it reads > like a tutorial as much as, if not more than guidelines. Would > separating the two make sense? I.e., have a standalone tutorial > page and just the most important high-level guidelines here? I'd prefer to have it as one ux.texi for now; if it gets too large we can split it up, but I'd prefer to combine the information in one place for now. > Besides the topics you already covered, some others that I think > would be useful to cover are those I mentioned at Cauldron: e.g., > how to choose where to implement a warning (pattern rules vs > flow-based rules), the common gotchas to look out for there, > etc. I'll be happy to contribute those once the first version > is agreed on and committed. Indeed, thanks. Dave > Martin > > > + > > +GCC's diagnostic subsystem can emit ``fix-it hints'': small > > suggested > > +edits to the user's source code. > > + > > +They are printed by default underneath the code in question. They > > +can also be viewed via @option{-fdiagnostics-generate-patch} and > > +@option{-fdiagnostics-parseable-fixits}. With the latter, an IDE > > +ought to be able to offer to automatically apply the suggested > > fix. > > + > > +Fix-it hints can be added to a diagnostic by using a > > @code{rich_location} > > +rather than a @code{location_t} - the fix-it hints are added to > > the > > +@code{rich_location} using one of the various @code{add_fixit} > > member > > +functions of @code{rich_location}. They are documented with > > +@code{rich_location} in @file{libcpp/line-map.h}. > > +It's easiest to use the @code{gcc_rich_location} subclass of > > +@code{rich_location} found in @file{gcc-rich-location.h}. > > + > > +For example: > > + > > +@smallexample > > + if (const char *suggestion = hint.suggestion ()) > > + @{ > > + gcc_rich_location richloc (location); > > + richloc.add_fixit_replace (suggestion); > > + error_at (&richloc, > > + "%qE does not name a type; did you mean %qs?", > > + id, suggestion); > > + @} > > +@end smallexample > > + > > +which can lead to: > > + > > +@smallexample > > +spellcheck-typenames.C:73:1: error: 'singed' does not name a type; > > did you mean 'signed'? > > +73 | singed char ch; > > + | ^~~~~~ > > + | signed > > +@end smallexample > > + > > +Non-trivial edits can be built up by adding multiple fix-it hints > > to one > > +@code{rich_location}. It's best to express the edits in terms of > > the > > +locations of individual tokens. Various handy functions for > > adding > > +fix-it hints for idiomatic C and C++ can be seen in > > +@file{gcc-rich-location.h}. > > + > > +@subsubsection Fix-it hints should be ``applyable'' > > + > > +When implementing a fix-it hint, please verify that the suggested > > edit > > +leads to fixed, compilable code. (Unfortunately, this currently > > must be > > +done by hand using @option{-fdiagnostics-generate-patch}. It > > would be > > +good to have an automated way of verifying that fix-it hints > > actually fix > > +the code). > > + > > +For example, a ``gotcha'' here is to forget to add a space when > > adding a > > +missing reserved word. Consider a C++ fix-it hint that adds > > +@code{typename} in front of a template declaration. A naive way > > to > > +implement this might be: > > + > > +@smallexample > > +gcc_rich_location richloc (loc); > > +// BAD: insertion is missing a trailing space > > +richloc.add_fixit_insert_before ("typename"); > > +error_at (&richloc, "need %<typename%> before %<%T::%E%> because " > > + "%qT is a dependent scope", > > + parser->scope, id, parser->scope); > > +@end smallexample > > + > > +When applied to the code, this might lead to: > > + > > +@smallexample > > +T::type x; > > +@end smallexample > > + > > +being ``corrected'' to: > > + > > +@smallexample > > +typenameT::type x; > > +@end smallexample > > + > > +In this case, the correct thing to do is to add a trailing space > > after > > +@code{typename}: > > + > > +@smallexample > > +gcc_rich_location richloc (loc); > > +// OK: note that here we have a trailing space > > +richloc.add_fixit_insert_before ("typename "); > > +error_at (&richloc, "need %<typename%> before %<%T::%E%> because " > > + "%qT is a dependent scope", > > + parser->scope, id, parser->scope); > > +@end smallexample > > + > > +leading to this corrected code: > > + > > +@smallexample > > +typename T::type x; > > +@end smallexample > > + > > +@subsubsection Express deletion in terms of deletion, not > > replacement > > + > > +It's best to express deletion suggestions in terms of deletion > > fix-it > > +hints, rather than replacement fix-it hints. For example, > > consider this: > > + > > +@smallexample > > + auto_diagnostic_group d; > > + gcc_rich_location richloc (location_of (retval)); > > + tree name = DECL_NAME (arg); > > + richloc.add_fixit_replace (IDENTIFIER_POINTER (name)); > > + warning_at (&richloc, OPT_Wredundant_move, > > + "redundant move in return statement"); > > +@end smallexample > > + > > +which is intended to e.g. replace a @code{std::move} with the > > underlying > > +value: > > + > > +@smallexample > > + return std::move (retval); > > + ~~~~~~~~~~^~~~~~~~ > > + retval > > +@end smallexample > > + > > +where the change has been expressed as replacement, replacing > > +with the name of the declaration. > > + > > +This works for simple cases, but consider this case: > > + > > +@smallexample > > +#ifdef SOME_CONFIG_FLAG > > +# define CONFIGURY_GLOBAL global_a > > +#else > > +# define CONFIGURY_GLOBAL global_b > > +#endif > > + > > +int fn () > > +@{ > > + return std::move (CONFIGURY_GLOBAL /* some comment */); > > +@} > > +@end smallexample > > + > > +The above implementation will erroneously strip out the macro and > > the > > +comment in the fix-it hint: > > + > > +@smallexample > > + return std::move (CONFIGURY_GLOBAL /* some comment */); > > + ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > + global_a > > +@end smallexample > > + > > +and thus this resulting code: > > + > > +@smallexample > > + return global_a; > > +@end smallexample > > + > > +It's better to do deletions in terms of deletions; deleting the > > +@code{std::move (} and the trailing close-paren, leading to > > +this: > > + > > +@smallexample > > + return std::move (CONFIGURY_GLOBAL /* some comment */); > > + ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > + CONFIGURY_GLOBAL /* some comment */ > > +@end smallexample > > + > > +and thus this result: > > + > > +@smallexample > > + return CONFIGURY_GLOBAL /* some comment */; > > +@end smallexample > > + > > +Unfortunately, the pertinent @code{location_t} values are not > > always > > +available. > > + > > +@c the above was https://gcc.gnu.org/ml/gcc-patches/2018-08/msg014 > > 74.html > > + > > +@subsubsection Multiple suggestions > > + > > +In the rare cases where you need to suggest more than one mutually > > +exclusive solution to a problem, this can be done by emitting > > +multiple notes and calling > > +@code{rich_location::fixits_cannot_be_auto_applied} on each note's > > +@code{rich_location}. If this is called, then the fix-it hints in > > +the @code{rich_location} will be printed, but will not be added to > > +generated patches. > > + > > + > > +@node Guidelines for Options > > +@cindex Options, Guidelines for > > +@cindex Guidelines for Options > > + > > +@section Guidelines for Options > > + > > +@c TODO > > > >
On 10/12/18 9:43 AM, David Malcolm wrote: > Here's a proposed "User Experience Guidelines" section for our > internals manual > > It's a mixture of proposed policy, together with notes on how to > implement the recommendations. > > Thoughts? > > gcc/ChangeLog: > * Makefile.in (TEXI_GCCINT_FILES): Add ux.texi. > * doc/gccint.texi: Include ux.texi and use it in top-level menu. > * doc/ux.texi: New file. I'll ACK with the explicit caveat that we may need to adjust the coding conventions (those are usually a wonderful discussion unto themselves). We shouldn't consider these written in stone at this time. I'll also note that the guidelines are a reasonable direction, they do not necessarily reflect current state everywhere. jeff
On 10/12/2018 09:43 AM, David Malcolm wrote: > Here's a proposed "User Experience Guidelines" section for our > internals manual > > It's a mixture of proposed policy, together with notes on how to > implement the recommendations. > > Thoughts? I think this documentation will be very helpful. I'll leave other people who've worked on this aspect of the code to comment on the content, but a few markup/copy-editing things I noticed while skimming the patch: - Can you please not use double-quote markup around so many words and phrases? If there's a technical term, use @dfn{} at the first use where you define it (and probably also an @cindex entry), and no markup on subsequent uses. In most other cases it seemed like the quotes would just be distracting from the flow of the text. - I don't think "end-user" should be hyphenated when used as a noun, although as an adjective phrase like "end-user experience" etc is fine. - Remember to use @noindent when continuing a sentence or paragraph broken up by a code example. I'll take a deeper dive on the next iteration of the patch. -Sandra
diff --git a/gcc/Makefile.in b/gcc/Makefile.in index 70efab7..3f05e95 100644 --- a/gcc/Makefile.in +++ b/gcc/Makefile.in @@ -3176,7 +3176,7 @@ TEXI_GCCINT_FILES = gccint.texi gcc-common.texi gcc-vers.texi \ gnu.texi gpl_v3.texi fdl.texi contrib.texi languages.texi \ sourcebuild.texi gty.texi libgcc.texi cfg.texi tree-ssa.texi \ loop.texi generic.texi gimple.texi plugins.texi optinfo.texi \ - match-and-simplify.texi poly-int.texi + match-and-simplify.texi ux.texi poly-int.texi TEXI_GCCINSTALL_FILES = install.texi install-old.texi fdl.texi \ gcc-common.texi gcc-vers.texi diff --git a/gcc/doc/gccint.texi b/gcc/doc/gccint.texi index 1a1af41..2554b31 100644 --- a/gcc/doc/gccint.texi +++ b/gcc/doc/gccint.texi @@ -125,6 +125,7 @@ Additional tutorial information is linked to from * LTO:: Using Link-Time Optimization. * Match and Simplify:: How to write expression simplification patterns for GIMPLE and GENERIC +* User Experience Guidelines:: Guidelines for implementing diagnostics and options. * Funding:: How to help assure funding for free software. * GNU Project:: The GNU Project and GNU/Linux. @@ -162,6 +163,7 @@ Additional tutorial information is linked to from @include plugins.texi @include lto.texi @include match-and-simplify.texi +@include ux.texi @include funding.texi @include gnu.texi diff --git a/gcc/doc/ux.texi b/gcc/doc/ux.texi new file mode 100644 index 0000000..87ff599 --- /dev/null +++ b/gcc/doc/ux.texi @@ -0,0 +1,455 @@ +@c Copyright (C) 2018 Free Software Foundation, Inc. +@c Free Software Foundation, Inc. +@c This is part of the GCC manual. +@c For copying conditions, see the file gcc.texi. + +@node User Experience Guidelines +@chapter User Experience Guidelines +@cindex User Experience Guidelines +@cindex Guidelines, User Experience + +To borrow a slogan from + @uref{https://elm-lang.org/blog/compilers-as-assistants, Elm}, + +@quotation +@strong{Compilers should be assistants, not adversaries.} A compiler should +not just detect bugs, it should then help you understand why there is a bug. +It should not berate you in a robot voice, it should give you specific hints +that help you write better code. Ultimately, a compiler should make +programming faster and more fun! +@author Evan Czaplicki +@end quotation + +This chapter provides guidelines on how to implement diagnostics and +command-line options in ways that we hope achieve the above ideal. + +@menu +* Guidelines for Diagnostics:: How to implement diagnostics. +* Guidelines for Options:: Guidelines for command-line options. +@end menu + + +@node Guidelines for Diagnostics +@cindex Guidelines for Diagnostics +@cindex Diagnostics, Guidelines for + +@section Guidelines for Diagnostics + +@subsection Talk in terms of the user's code + +Diagnostics should be worded in terms of the user's source code, and the +source language, rather than GCC's own implementation details. + +@subsection Diagnostics are ``actionable'' + +A good diagnostic is @emph{actionable}: it should assist the user in +taking action. + +Consider what an end-user will want to do when encountering a diagnostic. + +Given an error, an end-user will think: ``How do I fix this?'' + +Given a warning, an end-user will want to review the warning and think: + +@itemize @bullet +@item +``Is this a ``true'' result?'' +@item +``Do I care?'' +@item +if they decide it's genuine: ``How do I fix this?'' +@end itemize + +A good diagnostic provides pertinent information to allow the user to +easily answer the above questions. + +@subsection The user's attention is important + +A perfect compiler would issue a warning on every aspect of the user's +source code that ought to be fixed, and issue no other warnings. +Naturally, this ideal is impossible to achieve. + +Warnings should have a good ``signal:noise ratio'': we should have few +``false positives'' (falsely issuing a warning when no warning is +warranted) and few ``false negatives'' (failing to issue a warning when +one @emph{is} justified). + +Note that a ``false positive'' can mean, in practice, a warning that the +user doesn't agree with. Ideally a diagnostic should contain enough +information to allow the user to make an informed choice about whether +they should care (and how to fix it), but a balance must be drawn against +overloading the user with irrelevant data. + +It's worth testing a new warning on many instances of real-world code, +written by different people, and seeing what it complains about, and +what it doesn't complain about. This may suggest heuristics that +silence common false positives. + +@subsection Make mismatches clear + +Many diagnostics relate to a mismatch between two different places in the +user's source code. Examples include: +@itemize @bullet + @item + a type mismatch, where the type at a usage site does not match the type + at a declaration + + @item + the argument count at a call site does not match the parameter count + at the declaration + + @item + something is erroneously duplicated (e.g. an error, due to breaking a + uniqueness requirement, or a warning, if it's suggestive of a bug) + + @item + an ``opened'' syntactic construct (such as an open-parenthesis) is not + closed + + @c TODO: more examples? +@end itemize + +In each case, the diagnostic should indicate @strong{both} pertinent +locations (so that the user can easily see the problem and how to fix it). + +The standard way to do this is with a note (via @code{inform}). For +example: + +@smallexample + auto_diagnostic_group d; + if (warning_at (loc, OPT_Wduplicated_cond, + "duplicated %<if%> condition")) + inform (EXPR_LOCATION (t), "previously used here"); +@end smallexample + +For cases involving punctuation where the locations might be near +each other, they can be conditionally consolidated via +@code{gcc_rich_location::add_location_if_nearby}: + +@smallexample + auto_diagnostic_group d; + gcc_rich_location richloc (primary_loc); + bool added secondary = richloc.add_location_if_nearby (secondary_loc); + error_at (&richloc, "main message"); + if (!added secondary) + inform (secondary_loc, "message for secondary"); +@end smallexample + +This will emit either one diagnostic with two locations: +@smallexample + demo.c:42:10: error: main message + (foo) + ~ ^ +@end smallexample + +or two diagnostics: + +@smallexample + demo.c:42:4: error: main message + foo) + ^ + demo.c:40:2: note: message for secondary + ( + ^ +@end smallexample + +@subsection Location Information + +GCC's @code{location_t} type can support both ``ordinary'' locations, +and locations relating to a macro expansion. + +As of GCC 6, ordinary locations changed from supporting just a +point in the user's source code to supporting three points: the +``caret'' location, plus a start and a finish: + +@smallexample + a = foo && bar; + ~~~~^~~~~~ + | | | + | | finish + | caret + start +@end smallexample + +Tokens coming out of libcpp have locations of the form ``caret == start'', +such as for ``foo'' here: + +@smallexample + a = foo && bar; + ^~~ + | | + | finish + caret == start +@end smallexample + +Compound expressions should be reported using the location of the +expression as a whole, rather than just of one token within it. + +For example, in @code{-Wformat}, rather than underlining just the first +token of a bad argument: + +@smallexample + printf("hello %i %s", (long)0, "world); + ~^ ~ + %li +@end smallexample + +the whole of the expression should be underlined, so that the user can +easily identify what is being referred to: + +@smallexample + printf("hello %i %s", (long)0, "world); + ~^ ~~~~~~~ + %li +@end smallexample + +@c this was r251239 + +Avoid using the @code{input_location} global, and the diagnostic functions +that implicitly use it - use @code{error_at} and @code{warning_at} rather +than @code{error} and @code{warning}. + +@c TODO labelling of ranges + +@subsection Coding Conventions + +See the @uref{https://gcc.gnu.org/codingconventions.html#Diagnostics, +diagnostics section} of the GCC coding conventions. + +In the C++ frontend, when comparing two types in a message, use @code{%H} +and @code{%I} rather tha @code{%T}, as this allows the diagnostics +subsystem to highlight differences between template-based types. + +Use @code{auto_diagnostic_group} when issuing multiple related +diagnostics (seen in various examples on this page). This informs the +diagnostic subsystem that all diagnostics issued within the lifetime +of the @code{auto_diagnostic_group} are related. (Currently it doesn't +do anything with this information, but we may implement that in the +future). + +@subsection Spelling and Terminology + +See the @uref{https://gcc.gnu.org/codingconventions.html#Spelling +Spelling, terminology and markup} section of the GCC coding conventions. + +@subsection Tense of messages +Syntax errors occur in the present tense e.g. + +@smallexample +error_at (loc, "cannot convert %qH to %qI"); +@end smallexample + +and thus + +@smallexample +// CORRECT: usage of present tense: +error: cannot convert 'int' to 'void *' +@end smallexample + +rather than + +@smallexample +// BAD: usage of past tense: +error: could not convert 'int' to 'void *' +@end smallexample + +Predictions about run-time behavior should be described in the future tense e.g. + +@smallexample +warning_at (loc, OPT_some_warning, + "this code will read past the end of the array"); +@end smallexample + +@subsection Fix-it hints + +GCC's diagnostic subsystem can emit ``fix-it hints'': small suggested +edits to the user's source code. + +They are printed by default underneath the code in question. They +can also be viewed via @option{-fdiagnostics-generate-patch} and +@option{-fdiagnostics-parseable-fixits}. With the latter, an IDE +ought to be able to offer to automatically apply the suggested fix. + +Fix-it hints can be added to a diagnostic by using a @code{rich_location} +rather than a @code{location_t} - the fix-it hints are added to the +@code{rich_location} using one of the various @code{add_fixit} member +functions of @code{rich_location}. They are documented with +@code{rich_location} in @file{libcpp/line-map.h}. +It's easiest to use the @code{gcc_rich_location} subclass of +@code{rich_location} found in @file{gcc-rich-location.h}. + +For example: + +@smallexample + if (const char *suggestion = hint.suggestion ()) + @{ + gcc_rich_location richloc (location); + richloc.add_fixit_replace (suggestion); + error_at (&richloc, + "%qE does not name a type; did you mean %qs?", + id, suggestion); + @} +@end smallexample + +which can lead to: + +@smallexample +spellcheck-typenames.C:73:1: error: 'singed' does not name a type; did you mean 'signed'? +73 | singed char ch; + | ^~~~~~ + | signed +@end smallexample + +Non-trivial edits can be built up by adding multiple fix-it hints to one +@code{rich_location}. It's best to express the edits in terms of the +locations of individual tokens. Various handy functions for adding +fix-it hints for idiomatic C and C++ can be seen in +@file{gcc-rich-location.h}. + +@subsubsection Fix-it hints should be ``applyable'' + +When implementing a fix-it hint, please verify that the suggested edit +leads to fixed, compilable code. (Unfortunately, this currently must be +done by hand using @option{-fdiagnostics-generate-patch}. It would be +good to have an automated way of verifying that fix-it hints actually fix +the code). + +For example, a ``gotcha'' here is to forget to add a space when adding a +missing reserved word. Consider a C++ fix-it hint that adds +@code{typename} in front of a template declaration. A naive way to +implement this might be: + +@smallexample +gcc_rich_location richloc (loc); +// BAD: insertion is missing a trailing space +richloc.add_fixit_insert_before ("typename"); +error_at (&richloc, "need %<typename%> before %<%T::%E%> because " + "%qT is a dependent scope", + parser->scope, id, parser->scope); +@end smallexample + +When applied to the code, this might lead to: + +@smallexample +T::type x; +@end smallexample + +being ``corrected'' to: + +@smallexample +typenameT::type x; +@end smallexample + +In this case, the correct thing to do is to add a trailing space after +@code{typename}: + +@smallexample +gcc_rich_location richloc (loc); +// OK: note that here we have a trailing space +richloc.add_fixit_insert_before ("typename "); +error_at (&richloc, "need %<typename%> before %<%T::%E%> because " + "%qT is a dependent scope", + parser->scope, id, parser->scope); +@end smallexample + +leading to this corrected code: + +@smallexample +typename T::type x; +@end smallexample + +@subsubsection Express deletion in terms of deletion, not replacement + +It's best to express deletion suggestions in terms of deletion fix-it +hints, rather than replacement fix-it hints. For example, consider this: + +@smallexample + auto_diagnostic_group d; + gcc_rich_location richloc (location_of (retval)); + tree name = DECL_NAME (arg); + richloc.add_fixit_replace (IDENTIFIER_POINTER (name)); + warning_at (&richloc, OPT_Wredundant_move, + "redundant move in return statement"); +@end smallexample + +which is intended to e.g. replace a @code{std::move} with the underlying +value: + +@smallexample + return std::move (retval); + ~~~~~~~~~~^~~~~~~~ + retval +@end smallexample + +where the change has been expressed as replacement, replacing +with the name of the declaration. + +This works for simple cases, but consider this case: + +@smallexample +#ifdef SOME_CONFIG_FLAG +# define CONFIGURY_GLOBAL global_a +#else +# define CONFIGURY_GLOBAL global_b +#endif + +int fn () +@{ + return std::move (CONFIGURY_GLOBAL /* some comment */); +@} +@end smallexample + +The above implementation will erroneously strip out the macro and the +comment in the fix-it hint: + +@smallexample + return std::move (CONFIGURY_GLOBAL /* some comment */); + ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + global_a +@end smallexample + +and thus this resulting code: + +@smallexample + return global_a; +@end smallexample + +It's better to do deletions in terms of deletions; deleting the +@code{std::move (} and the trailing close-paren, leading to +this: + +@smallexample + return std::move (CONFIGURY_GLOBAL /* some comment */); + ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + CONFIGURY_GLOBAL /* some comment */ +@end smallexample + +and thus this result: + +@smallexample + return CONFIGURY_GLOBAL /* some comment */; +@end smallexample + +Unfortunately, the pertinent @code{location_t} values are not always +available. + +@c the above was https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01474.html + +@subsubsection Multiple suggestions + +In the rare cases where you need to suggest more than one mutually +exclusive solution to a problem, this can be done by emitting +multiple notes and calling +@code{rich_location::fixits_cannot_be_auto_applied} on each note's +@code{rich_location}. If this is called, then the fix-it hints in +the @code{rich_location} will be printed, but will not be added to +generated patches. + + +@node Guidelines for Options +@cindex Options, Guidelines for +@cindex Guidelines for Options + +@section Guidelines for Options + +@c TODO