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®rtested 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
