On Fri, Jan 13, 2017 at 5:05 PM, David Malcolm <dmalc...@redhat.com> wrote: > On Wed, 2017-01-04 at 14:58 -0500, Jason Merrill wrote: >> On Tue, Jan 3, 2017 at 8:28 PM, David Malcolm <dmalc...@redhat.com> >> wrote: >> > PR c++/77829 and PR c++/78656 identify an issue within the C++ >> > frontend >> > where it issues nonsensical fix-it hints for misspelled name >> > lookups >> > within an explicitly given namespace: it finds the closest name >> > within >> > all namespaces, and uses the location of the namespace for the >> > replacement, >> > rather than the name. >> > >> > For example, for this error: >> > >> > #include <memory> >> > void* allocate(std::size_t n) >> > { >> > return std::alocator<char>().allocate(n); >> > } >> > >> > we currently emit an erroneous fix-it hint that would generate this >> > nonsensical patch: >> > >> > { >> > - return std::alocator<char>().allocate(n); >> > + return allocate::alocator<char>().allocate(n); >> > } >> > >> > whereas we ought to emit a fix-it hint that would generate this >> > patch: >> > >> > { >> > - return std::alocator<char>().allocate(n); >> > + return std::allocator<char>().allocate(n); >> > } >> > >> > This patch fixes the suggestions, in two parts: >> > >> > The incorrect name in the suggestion is fixed by introducing a >> > new function "suggest_alternative_in_explicit_scope" >> > for use by qualified_name_lookup_error when handling failures >> > in explicitly-given namespaces, looking for hint candidates within >> > just that namespace. The function suggest_alternatives_for gains a >> > "suggest_misspellings" bool param, so that we can disable fuzzy >> > name >> > lookup for the case where we've ruled out hint candidates in the >> > explicitly-given namespace. >> > >> > This lets us suggest "allocator" (found in "std") rather "allocate" >> > (found in the global ns). >> >> This looks good. >> >> > The patch fixes the location for the replacement by updating >> > local "unqualified_id" in cp_parser_id_expression from tree to >> > cp_expr to avoid implicitly dropping location information, and >> > passing this location to a new param of finish_id_expression. >> > There are multiple users of finish_id_expression, and it wasn't >> > possible to provide location information for the id for all of them >> > so the new location information is assumed to be optional there. >> >> > This fixes the underlined location, and ensures that the fix-it >> > hint >> > replaces "alocator", rather than "std". >> >> I'm dubious about this approach, as I think this will fix some cases >> and not others. The general problem is that we aren't sure what to >> do >> with the location of a qualified-id: sometimes we use the location of >> the unqualified-id, sometimes we use the beginning of the first >> nested-name-specifier. I think the right answer is probably to use a >> rich location with the caret on the unqualified-id. Then the fix-it >> hint can use the caret location for the fix-it. Does that make >> sense? > > Sorry, I'm not sure I follow you. > > By "rich location" do you mean providing multiple ranges (e.g. one for > the nested-name-specifier, another for the unqualified-id)? > > e.g. > > ::foo::bar > ~~~ ^~~ > | | > | `-(primary location and range) > `-(secondary range) > > or: > > ::foo::bar > ~~~~~ ^~~ > | | > | `-(primary location and > range) > `-(secondary range) > > (if that ASCII art makes sense) > > Note that such a rich location (with more than one range) can only > exist during the emission of a diagnostic; it's not something we can > use as the location of a tree node. > > Or do you mean that we should supply a range for the unqualified-id, > with the caret at the start of the unqualified-id, e.g.: > > ::foo::bar > ^~~ > > (a tree node code can have such a location). > > It seems to me that we ought to have > > ::foo::bar > ^~~~~~~~~~ > > as the location of such a tree node, and only use the range of just > "bar" as the location when handling name lookup errors (such as when > emitting fix-it hints).
Yes, sorry, I meant range. I was thinking ::foo::bar ~~~~~~~^~~ And then the fix-it would know to replace from the caret to the end of the range? Jason