[PING] Re: [PATCH] c/c++: Add fix-it hints for suggested missing #includes

[David Malcolm]

On Fri, 2017-06-30 at 09:40 -0600, Jeff Law wrote:
> On 05/26/2017 01:54 PM, David Malcolm wrote:
> > Ping:
> >   https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00321.html
> > 
> > On Thu, 2017-05-04 at 12:36 -0400, David Malcolm wrote:
> > > As of r247522, fix-it-hints can suggest the insertion of new
> > > lines.
> > > 
> > > This patch uses this to implement a new "maybe_add_include_fixit"
> > > function in c-common.c and uses it in the two places where the C
> > > and
> > > C++
> > > frontend can suggest missing #include directives. [1]
> > > 
> > > The idea is that the user can then click on the fix-it in an IDE
> > > and have it add the #include for them (or use -fdiagnostics
> > > -generate
> > > -patch).
> > > 
> > > Examples can be seen in the test cases.
> > > 
> > > The function attempts to put the #include in a reasonable place:
> > > immediately after the last #include within the file, or at the
> > > top of the file.  It is idempotent, so -fdiagnostics-generate
> > > -patch
> > > does the right thing if several such diagnostics are emitted.
> > > 
> > > Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
> > > 
> > > OK for trunk?
> > > 
> > > [1] I'm working on a followup which tweaks another diagnostic so
> > > that
> > > it
> > > can suggest that a #include was missing, so I'll use it there as
> > > well.
> > > 
> > > gcc/c-family/ChangeLog:
> > > 	* c-common.c (try_to_locate_new_include_insertion_point): New
> > > 	function.
> > > 	(per_file_includes_t): New typedef.
> > > 	(added_includes_t): New typedef.
> > > 	(added_includes): New variable.
> > > 	(maybe_add_include_fixit): New function.
> > > 	* c-common.h (maybe_add_include_fixit): New decl.
> > > 
> > > gcc/c/ChangeLog:
> > > 	* c-decl.c (implicitly_declare): When suggesting a missing
> > > 	#include, provide a fix-it hint.
> > > 
> > > gcc/cp/ChangeLog:
> > > 	* name-lookup.c (get_std_name_hint): Add '<' and '>' around
> > > 	the header names.
> > > 	(maybe_suggest_missing_header): Update for addition of '<' and
> > > '>'
> > > 	to above.  Provide a fix-it hint.
> > > 
> > > gcc/testsuite/ChangeLog:
> > > 	* g++.dg/lookup/missing-std-include-2.C: New text case.
> > > 	* gcc.dg/missing-header-fixit-1.c: New test case.
> Generally OK.  But a few comments on how you find the location for
> where
> to suggest the new #include.

Thanks for looking at it.

> It looks like you're walking the whole table every time?!?

Note that try_to_locate_new_include_insertion_point is guarded by the
idempotency logic within maybe_add_include_fixit: it will be called at
most once per suggested #include.

I don't know if "walking" is the word I would have used: it's iterating
over the whole array of structs, doing a few pointer compares of fields
within it.  The elements in the array are line *maps*, not lines
themselves, so it's likely to be of the order of a few thousand entries
for a source file with plenty of nested #includes.

> Shouldn't
> you at least bound things between start of file 

By "start of file", do you mean "start of the main source file", or the
start of the file containing the diagnostic?  AIUI, getting at this
information requires a walk forwards through the line maps, which is
exactly what the patch is doing.

> and the point where an
> error was issued?  ie, if you used an undefined type on line XX, a
> #include after line XX makes no sense to resolve the error.

Hmmm.  I agree that the fix-it hint would makes little sense for this
case.  I did some testing of this case, and it uncovered some bugs with
the patch, so I'm going to post an updated patch, with some test
coverage for it (and addressing the bugs).

But I'd prefer to focus on correctness rather than on performance here;
because of the idempotency guard it's only called once per header file
that we "know" about (and can thus suggest in a fix-it hint).

> I'm not sure how often this will get called when someone does
> something
> stupid and needs the #include.  But ISTM you're looking for two
> bounds.
> 
> For the "last" case you start at the statement which generated the
> error
> and walk backwards stopping when you find the last map.

It's not clear to me what you're proposing.  If we expand the
statement's location to get a physical location, that will be within an
ordinary line_map, within the file in question.  The start of that
linemap might related to returning from a #include, but it might also
relate to changes in line length (due to cpplib trying to make best use
of the 32-bit encoding space) e.g. in the middle of a function, or in
the middle of a comment.

We could maybe use this ordinary map as an upper boundary for the
search (though I'd want to check that the logic in the search still
makes sense for this case).

> For the "first" case, you start at the beginning and walk forward to
> find the map, then quit.

Again, I'm not quite sure what you mean here.

> Are those not appropriate here?

As noted above, these feel to me like premature optimizations; I don't
see this as a performance hog, so I'd prefer to focus on getting the
implementation correct (and simple) first.

I'm working on an updated patch fixing the bugs mentioned above; does
the above address your concerns about v1 of the patch for now?

As a further followup, consider the case of a diagnostic about a
missing #include emitted within a header: if we're adding the #include
to the header (which is what the patch does), then maybe we should put
the suggested #include inside #ifndef guards if they're present, rather
than at the top of the header.  I think I've now figured out how to
express that in DejaGnu :)  Does that sound worth pursuing in this
patch, or in a followup?

Thanks
Dave