On Wed, 2016-08-24 at 09:59 +0200, Richard Biener wrote:
> On Wed, Aug 24, 2016 at 3:28 AM, David Malcolm <dmalc...@redhat.com>
> wrote:
> > Currently our diagnostics subsystem can emit fix-it hints
> > suggesting edits the user can make to their code to fix
> > warnings/errors,
> > but currently the user must make these edits manually [1].
> >
> > The following patch kit adds two command-line options with which
> > we can make the changes on behalf of the user:
> >
> > (A) -fdiagnostics-generate-patch emits a diff to stderr, which
> > could potentially be applied using "patch"; it also gives another
> > way to visualize the fix-its.
> >
> > (B) -fdiagnostics-apply-fixits, which writes back the changes
> > to the input files.
>
> I don't like this very much - option (A) looks good with the user
> having
> to manually apply a patch. But (B) is just asking for trouble ;)
Yeah, the current implementation of (B) has issues with atomicity; I
wonder what would happen in a parallel build if a fix-it happened
affecting a header file...
Would you be open to a less ambitious version of the patch kit which
just implemented (A)?
> I'd rather have -fdiagnostics-apply-fixits apply the fixit hints for
> the current compilation only (so you'll see if the compile is
> successful
> after applying the suggestions). That is, work more in a "error
> recovery"
> mode.
> Richard.
One approach here would be to provide a small binary that knows how to
parse the output of -fdiagnostics-parseable-fixits, and applies them,
something like ${bindir}/gcc-apply-fixits
It would read a stream of text on stdin (taken from stderr of cc1 and
friends), parse any lines emitted by -fdiagnostics-parseable-fixits
(ignoring the rest), put them all into an edit_context, and then apply
them (or generate a patch).
If nothing else, this might be useful for people integrating fix-its
into an IDE.
I was thinking that the driver could then have an option to attempt the
compile, and have it send stderr through the binary, and somehow run
the compiler twice. But presumably we'd want to have the 2nd
invocation of the compiler be run on temporarily-modified sources; what
happens if we need to issue a diagnostic on such sources? In
particular what filename does the user see? It seems that we'd need
some way of mapping filenames, so that the compiler and the user are
effectively seeing an "as-if" view of the filesystem (as if the changes
were applied).
Alternatively, maybe this is too implementation-focused: we should
start by thinking of the interaction model - what do we want the user's
experience to be?
I wonder if there's a way for a fix-it to actually change the token
stream seen by the compiler (doing it all in one invocation of the
compiler).
Dave
> > Patches 1 and 2 are enabling work.
> >
> > Patch 3 is the heart of the kit: a new class edit_context,
> > representing a set of changes to be applied to source file(s).
> > The changes are atomic: all are applied or none [2], and locations
> > are
> > specified relative to the initial inputs (and there's some fiddly
> > logic for fixing up locations so that the order in which edits are
> > applied doesn't matter).
> >
> > Patch 4 uses the edit_context class to add the new options.
> >
> > The kit also unlocks the possibility of writing refactoring tools
> > as gcc plugins, or could be used to build a parser for the output
> > of -fdiagnostics-parseable-fixits.
> >
> > Successfully bootstrapped®rtested the combination of the patches
> > on x86_64-pc-linux-gnu.
> >
> > I believe I can self-approve patch 3, and, potentially patch 4
> > as part of my "diagnostics maintainer" role, though this is
> > clearly a significant extension of the scope of diagnostics.
> >
> > OK for trunk? (assuming individual bootstraps®rtesting)
> >
> > [1] or use -fdiagnostics-parseable-fixits - but we don't have a
> > way to parse that output format yet
> > [2] the error-handling for write-back isn't great right now, so
> > the claim of atomicity is a stretch; is there a good cross-platform
> > way to guarantee atomicity of a set of filesystem operations?
> > (maybe create the replacements as tempfiles, rename the originals,
> > try to move all the replacements into place, and then unlink the
> > backups, with a rollback involving moving the backups into place)
> >
> > David Malcolm (4):
> > selftest: split out named_temp_file from temp_source_file
> > Improvements to typed_splay_tree
> > Introduce class edit_context
> > Add -fdiagnostics-generate-patch and -fdiagnostics-apply-fixits
> >
> > gcc/Makefile.in | 2 +
> > gcc/common.opt | 8 +
> > gcc/diagnostic-color.c | 8 +-
> > gcc/diagnostic.c | 11 +
> > gcc/diagnostic.h | 7 +
> > gcc/doc/invoke.texi | 28 +-
> > gcc/edit-context.c | 1341
> > ++++++++++++++++++++
> > gcc/edit-context.h | 66 +
> > gcc/selftest-run-tests.c | 2 +
> > gcc/selftest.c | 49 +-
> > gcc/selftest.h | 26 +-
> > .../diagnostic-test-show-locus-generate-patch.c | 77 ++
> > gcc/testsuite/gcc.dg/plugin/plugin.exp | 3 +-
> > gcc/toplev.c | 20 +
> > gcc/typed-splay-tree.c | 79 ++
> > gcc/typed-splay-tree.h | 67 +
> > 16 files changed, 1770 insertions(+), 24 deletions(-)
> > create mode 100644 gcc/edit-context.c
> > create mode 100644 gcc/edit-context.h
> > create mode 100644 gcc/testsuite/gcc.dg/plugin/diagnostic-test
> > -show-locus-generate-patch.c
> > create mode 100644 gcc/typed-splay-tree.c
> >
> > --
> > 1.8.5.3
> >
