[PATCH] v2: gccint.texi: add user experience guidelines
David Malcolm
dmalcolm@redhat.com
Thu Oct 18 21:08:00 GMT 2018
On Tue, 2018-10-16 at 21:39 -0600, Sandra Loosemore 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?
>
> 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
Thanks.
Here's an updated version of the patch, addressing your above comments,
and those from Martin and Richard (I hope).
I have a couple of texinfo questions:
(a) the guidelines frequently have contrasting pairs
of examples showing how to do something vs how not to do it. Is there
a way of marking these up in texinfo beyond just @smallexample?
(and manually putting in "BAD" and "OK", as I've done)
(b) what's the best way of showing example output from gcc? In
particular I wasn't able to properly express the single quotes emitted by
GCC's %qs, %<, and %> directives: everything I've tried so far has issues
in at least one of the pdf vs the html output. I've settled for using
single quotes, which is easy to emit via LANG=C and looks OK in html,
but less good in pdf. (also, I'd love to be able to express colorization
sanely; see e.g.:
https://gcc.gnu.org/ml/gcc-patches/2018-04/msg00186.html
which takes colorized GCC output and turns it into HTML for our
website; e.g. gcc-8/changes.html).
Changed in v2:
* removed "Tense of Messages" section for now
* simplified "Given a warning, an end-user will want to review the
warning and think:" to "Given a warning, an end-user will think:"
* reworded "Is this a ``true'' result?" to "Is this a real problem?"
* split out the testing idea into a "Try the diagnostic on real-world
code" subsection and added not about precision of wording
* added example of output for OPT_duplicated_cond example
* added note about guarding the "inform"
* added example of using %H and %I
* added @noindent in many places; merged some paragraphs
* "end-user" -> "end user" in the 3 places it was used
* replaced many uses of double-quotes with @dfn{}, adding @cindex
* converted @cindex to lower-case
* fixed string literal syntax in -Wformat example
* added "Precision of Wording" section, and used the same example
to illustrate the "don't use input_location" guideline.
* added more "// BAD" and "// OK" lines to examples
* fixed "are and" typo
* give rationale for why gcc_rich_location is preferable to
rich_location
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 | 595 ++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 598 insertions(+), 1 deletion(-)
create mode 100644 gcc/doc/ux.texi
diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 8a85d7e..e649b9d 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -3177,7 +3177,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..12d5fb8
--- /dev/null
+++ b/gcc/doc/ux.texi
@@ -0,0 +1,595 @@
+@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
+@cindex diagnostics, actionable
+
+A good diagnostic is @dfn{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 think:
+
+@itemize @bullet
+@item
+``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.
+
+@cindex signal-to-noise ratio (metaphorical usage for diagnostics)
+@cindex diagnostics, false positive
+@cindex diagnostics, true positive
+@cindex false positive
+@cindex true positive
+
+Warnings should have a good @dfn{signal-to-noise ratio}: we should have few
+@dfn{false positives} (falsely issuing a warning when no warning is
+warranted) and few @dfn{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.
+
+@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
+demo.c:1:1: warning: 'noinline' attribute ignored [-Wattributes]
+ 1 | int foo __attribute__((noinline));
+ | ^~~
+@end smallexample
+
+@noindent
+doesn't tell the user why the attribute was ignored, or what kind of
+entity the compiler thought the attribute was being applied to (the
+source location for the diagnostic is also poor;
+@pxref{input_location_example,,discussion of @code{input_location}}).
+A better message would be:
+
+@smallexample
+demo.c:1:24: warning: attribute 'noinline' on variable 'foo' was ignored [-Wattributes]
+ 1 | int foo __attribute__((noinline));
+ | ~~~ ~~~~~~~~~~~~~~~^~~~~~~~
+demo.c:1:24: note: attribute 'noinline' is only applicable to functions
+@end smallexample
+
+@noindent
+which spells out the missing information (and fixes the location
+information, as discussed below).
+
+The above example uses a note to avoid a combinatorial explosion of possible
+messages.
+
+@subsection Try the diagnostic on real-world code
+
+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.
+
+It may also suggest ways to improve the precision of the message.
+
+@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
+
+@noindent
+which leads to:
+
+@smallexample
+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)
+ | ~~~~~^~~
+@end smallexample
+
+@noindent
+The @code{inform} call should be guarded by the return value from the
+@code{warning_at} call so that the note isn't emitted when the warning
+is suppressed.
+
+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
+
+@noindent
+This will emit either one diagnostic with two locations:
+@smallexample
+ demo.c:42:10: error: main message
+ (foo)
+ ~ ^
+@end smallexample
+
+@noindent
+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
+@cindex diagnostics, locations
+@cindex location information
+@cindex source code, location information
+@cindex caret
+
+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
+@dfn{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 @code{caret == start},
+such as for @code{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
+
+@noindent
+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}, and provide the most appropriate
+@code{location_t} value available at that phase of the compilation. It's
+possible to supply secondary @code{location_t} values via
+@code{rich_location}.
+
+@noindent
+@anchor{input_location_example}
+For example, in the example of imprecise wording
+above, the diagnostic was generated using @code{warning}:
+
+@smallexample
+ // BAD: implicitly uses @code{input_location}
+ warning (OPT_Wattributes, "%qE attribute ignored", name);
+@end smallexample
+
+@noindent
+leading to
+
+@smallexample
+// BAD: uses @code{input_location}
+demo.c:1:1: warning: 'noinline' attribute ignored [-Wattributes]
+ 1 | int foo __attribute__((noinline));
+ | ^~~
+@end smallexample
+
+@noindent
+which thus happened to use the location of the @code{int} token, rather
+than that of the attribute. Using @code{warning_at} with the location of
+the attribute, providing the location of the declaration in question
+as a secondary location, and adding a note:
+
+@smallexample
+ auto_diagnostic_group d;
+ gcc_rich_location richloc (attrib_loc);
+ richloc.add_range (decl_loc);
+ if (warning_at (OPT_Wattributes, &richloc,
+ "attribute %qE on variable %qE was ignored", name))
+ inform (attrib_loc, "attribute %qE is only applicable to functions");
+@end smallexample
+
+@noindent
+would lead to:
+
+@smallexample
+// OK: use location of attribute, with a secondary location
+demo.c:1:24: warning: attribute 'noinline' on variable 'foo' was ignored [-Wattributes]
+ 1 | int foo __attribute__((noinline));
+ | ~~~ ~~~~~~~~~~~~~~~^~~~~~~~
+demo.c:1:24: note: attribute 'noinline' is only applicable to functions
+@end smallexample
+
+@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.
+For example, rather than using @code{%qT}:
+
+@smallexample
+ // BAD: a pair of %qT used in C++ frontend for type comparison
+ error_at (loc, "could not convert %qE from %qT to %qT", expr,
+ TREE_TYPE (expr), type);
+@end smallexample
+
+@noindent
+which could lead to:
+
+@smallexample
+error: could not convert 'map<int, double>()' from 'map<int,double>' to 'map<int,int>'
+@end smallexample
+
+@noindent
+using @code{%H} and @code{%I} (via @code{%qH} and @code{%qI}):
+
+@smallexample
+ // OK: compare types in C++ frontend via %qH and %qI
+ error_at (loc, "could not convert %qE from %qH to %qI", expr,
+ TREE_TYPE (expr), type);
+@end smallexample
+
+@noindent
+allows the above output to be simplified to:
+
+@smallexample
+error: could not convert 'map<int, double>()' from 'map<[...],double>' to 'map<[...],int>'
+@end smallexample
+
+@noindent
+where the @code{double} and @code{int} are colorized to highlight them.
+
+@c %H and %I were added in r248698.
+
+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 Fix-it hints
+@cindex fix-it hints
+@cindex diagnostics guidelines, fix-it hints
+
+GCC's diagnostic subsystem can emit @dfn{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}, as this
+implicitly supplies the @code{line_table} variable.
+
+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
+
+@noindent
+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
+
+@noindent
+When applied to the code, this might lead to:
+
+@smallexample
+T::type x;
+@end smallexample
+
+@noindent
+being ``corrected'' to:
+
+@smallexample
+typenameT::type x;
+@end smallexample
+
+@noindent
+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
+
+@noindent
+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
+
+@noindent
+which is intended to e.g. replace a @code{std::move} with the underlying
+value:
+
+@smallexample
+ return std::move (retval);
+ ~~~~~~~~~~^~~~~~~~
+ retval
+@end smallexample
+
+@noindent
+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
+
+@noindent
+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
+
+@noindent
+and thus this resulting code:
+
+@smallexample
+ return global_a;
+@end smallexample
+
+@noindent
+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
+
+@noindent
+and thus this result:
+
+@smallexample
+ return CONFIGURY_GLOBAL /* some comment */;
+@end smallexample
+
+@noindent
+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 command-line options, guidelines for
+@cindex options, guidelines for
+@cindex guidelines for options
+
+@section Guidelines for Options
+
+@c TODO
--
1.8.5.3
More information about the Gcc-patches
mailing list