Re: [PATCH v2 2/2] gdb/c++: Detect ambiguous variables in imported namespaces

[Lancelot SIX <lsix@lancelotsix.com>]

> > > +      std::string error_str = "Reference to \"";
> > > +      error_str += name;
> > > +      error_str += "\" is ambiguous, possibilities are: ";
> > > +      error_str += itr->second.symbol->print_name ();
> > > +      for (itr++; itr != found_symbols.end (); itr++)
> > > +	{
> > > +	  error_str += " and ";
> > > +	  error_str += itr->second.symbol->print_name ();
> > > +	}
> > > +      error (_("%s"), error_str.c_str ());
> > I do not know if this something we care about, but your error message
> > will list the options in an unspecified order.  Iterating over the map
> > will follow the order of the keys, which are the addresses of the
> > mangled names of the symbols.
> > 
> > My instinct would prefer to list the options in lexicographical order
> > (of the demangled name).  This is probably not really important but it
> > could be annoying when writing test checking for the output message.
> 
> Would you be OK with lexicological ordering of mangled names? That could be
> easily done by changing to std::map<std::string, T> and we keep all of the
> rest of the code.
> 

Yes, that sounds OK to me.  This is what I had in mind ;)

> > >       }
> > > -  return result;
> > > +  if (result.empty ())
> > > +    return {};
> > > +  else
> > > +    return result.begin ()->second;
> > … here!
> > 
> > This however raises the question: if cp_lookup_symbol_via_imports can
> > return multiple block_symbol, shouldn't
> > cp_lookup_symbol_imports_or_template do the same when it basically
> > forwards what cp_lookup_symbol_via_imports returns?
> 
> Well, you just found a different flaw in my logic... My plan is that the
> function will only return multiple values when being called recursively, and
> the top-level call would error out in case it found multiple symbols. This
> makes it so asserting that the size is 1 or reacting to different sizes
> redundant outside cp_lookup_symbol_via_imports.
> 
> However, I forgot to add the check for the function being top level (seeing
> if search_scope_first is 1), which could lead us to incomplete lists. I'll
> fix this and document this behavior better for v3
> 
> > 
> > I am not really familiar with this part of the code, so I do not know if
> > this can happen (but I guess it could).
> > 
> > I would say that:
> > - If the map can only contain one element and you only introduce it to
> >    hold the result of cp_lookup_symbol_via_imports, then you should
> >    assert that its size is 1 here (and maybe have a comment explaining
> >    why it must be a map of size 1).
> As I mentioned above, before returning from the top-level call,
> cp_lookup_symbol_via_imports will already have errored out if there is more
> than one element. If you think it is important, I can document it. It felt
> self-explanatory to me, but I might just be in too deep at this point
> > - If it can be anything else, I guess you would need to return the
> >    entire map, or have a comment explaining why any member could do.
> > 
> > As the function's comment says "Like cp_lookup_symbol_via_imports, but
> > […]" I am tempted to think that it should also return a map.
> 
> I am thinking that maybe that comment should be rewritten. The reason for
> the call may be similar, but the internal workings look pretty different
> (mainly because it doesn't implement anything from
> cp_lookup_symbol_via_imports, just calls that function). I'm thinking
> something along the lines of:
> 
> search for symbols whose names match NAME. if BLOCK is a function, search
> through the template parameters and function type. Then (or if it is not a
> function) call cp_lookup_symbol_via imports to search for other symbols in
> the given SCOPE.
> 
> Or something of the sorts. What do you think?

Yeah, rephrasing the comment could help.

Best,
Lancelot.