Re: [PATCH 0/4] Applying fix-its on behalf of the user to their code

[Eric Gallager <egall at gwmail dot gwu dot edu>]

On 8/23/16, David Malcolm <dmalcolm@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.
>
> 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&regrtested 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&regrtesting)
>
> [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
>
>


So, if -fdiagnostics-apply-fixits writes the fixits back into the file
as actual code, I'm wondering, could there also be a separate option
to write them in as comments? Say if I want to be reminded to fix
something, but am not quite sure if the fixit is right or not, and
don't want to have a separate patch file lying around... e.g., instead
of changing
    return ptr.x;
to
    return ptr->x;
it could change it to
    return ptr.x; // ->
or maybe put it below like:
    return ptr.x;
               /* -> */
Just an idea I thought might be useful as a user... I dunno...