From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (lndn.lancelotsix.com [51.195.220.111]) by sourceware.org (Postfix) with ESMTPS id 9B34F384F6C7 for ; Thu, 17 Nov 2022 22:07:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9B34F384F6C7 Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=lancelotsix.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=lancelotsix.com Received: from ubuntu.lan (unknown [IPv6:2a02:390:9086::635]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 2732F80363; Thu, 17 Nov 2022 22:07:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=lancelotsix.com; s=2021; t=1668722872; bh=kQqg8o10J7WrEVw2uzadfcdzGVyBptXUn8VRuPL6AJQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=wA7UNVknCVrc6+nttCMaqm/pfbSpdrxFZz+CEfNUwX4mOVmPUBYEP2XgU2uBZ+zDa 96x9C8nyczJ/1vKogXO5/JCJO62AmsDk/9kCubjckUsht9Y+5VRwCsYUh1lxeqb9pa Up1fHimGSdRbvTbFapSgLJy/Tev5Wo8ZXPZkCvajPyIcQkEsZk/LLTjYxZouiEkRg8 akAhGBFdfhMh3oQhjU1wm8u1hk7Nz9njtasIMan2Q22gUVaN6EvFDk2DBQeOVBilze j/LTAHkr33BZIgEYwFo11we3b3kyx4V5/A0hRnP8PCpUicn866VWFiy2XzKH9K3hS/ z8KgIpuqGxzFw== Date: Thu, 17 Nov 2022 22:07:45 +0000 From: Lancelot SIX To: Bruno Larsen Cc: gdb-patches@sourceware.org, tom@tromey.com Subject: Re: [PATCH v2 2/2] gdb/c++: Detect ambiguous variables in imported namespaces Message-ID: <20221117220745.abwlijcztuayl255@ubuntu.lan> References: <20221116141336.1160869-1-blarsen@redhat.com> <20221116141336.1160869-3-blarsen@redhat.com> <20221116174913.hd3l6yyh5z6w7n7y@ubuntu.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Thu, 17 Nov 2022 22:07:52 +0000 (UTC) X-Spam-Status: No, score=-5.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: > > > + 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 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.