On 10/12/18, David Malcolm <dmalc...@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
>
>
