From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id EDBF23858004 for ; Fri, 5 Nov 2021 18:27:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EDBF23858004 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-163-npeJ8YhUO0iltW-5GE7rTw-1; Fri, 05 Nov 2021 14:27:14 -0400 X-MC-Unique: npeJ8YhUO0iltW-5GE7rTw-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AB68684BA63; Fri, 5 Nov 2021 18:26:54 +0000 (UTC) Received: from [10.97.116.50] (ovpn-116-50.gru2.redhat.com [10.97.116.50]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6572419C79; Fri, 5 Nov 2021 18:26:52 +0000 (UTC) Message-ID: <131933fc-d3f7-bd3c-a7a0-74d73eb22782@redhat.com> Date: Fri, 5 Nov 2021 15:26:49 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH] PR gdb/28480: Improve ambiguous member detection To: Simon Marchi , gdb-patches@sourceware.org References: <20211104210457.35512-1-blarsen@redhat.com> <0e7f3dbb-ef25-9d60-42da-b01c57eb8efa@simark.ca> From: Bruno Larsen In-Reply-To: <0e7f3dbb-ef25-9d60-42da-b01c57eb8efa@simark.ca> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-14.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 05 Nov 2021 18:27:19 -0000 On 11/5/21 14:54, Simon Marchi wrote: > On 2021-11-04 5:04 p.m., Bruno Larsen via Gdb-patches wrote: > snip >> diff --git a/gdb/testsuite/gdb.cp/ambiguous.cc b/gdb/testsuite/gdb.cp/ambiguous.cc >> index a55686547f2..b2be7297b28 100644 >> --- a/gdb/testsuite/gdb.cp/ambiguous.cc >> +++ b/gdb/testsuite/gdb.cp/ambiguous.cc >> @@ -1,3 +1,4 @@ >> +class empty { }; >> >> class A1 { >> public: >> @@ -17,6 +18,11 @@ public: >> int y; >> }; >> >> +class A4 { >> +public: >> + [[no_unique_address]] empty x; > > In theory, no_unique_address appeared in C++20. Having > [[no_unique_address]] in the source code means that compiling this file > with an older compiler that doesn't know about it, or in a standard mode > where it isn't recognized, will lead to the entire test being skipped. > In practice, it looks like older-but-not-too-old g++s cope well with it. > For example, g++ 7 prints a warning: > > $ g++-7 ambiguous.cc > ambiguous.cc:23:33: warning: ‘no_unique_address’ attribute directive ignored [-Wattributes] > [[no_unique_address]] empty x; > ^ > > ... but since we disable all warnings in that tests for g++ < 10, then > it still compiles. The no_unique_address attribute has no effect, but > that's fine. > > With older g++s, like 4.8, it fails though: > > $ make check TESTS="gdb.cp/ambiguous.exp" RUNTESTFLAGS='CC_FOR_TARGET=gcc-4.8 CXX_FOR_TARGET=g++-4.8' > ... > Running /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.cp/ambiguous.exp ... > Gdb compile failed, /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.cp/ambiguous.cc:23:5: error: expected unqualified-id before '[' token > [[no_unique_address]] empty x; > ^ > > I don't know if others care about this test running with older > compilers, but if so we could make that case optional with some > preprocessor conditionals. Alright, I'll wait and see if anyone says anything about this before pushing. I'll also address everything else you mentioned. > snip >> + { >> + bool ambiguous = false, insert = true; >> + for(auto finds: m_fields){ > > Space before opening parentheses and after closing. > > Iterate using const-reference, to avoid copying the items as we are > iterating. I would also use the type name instead of auto, for clarity, > but I am ok with either. Lastly, the variable name "finds" is a bit > strange, or I don't understand what you mean by it. I first named it found_field, but I dont like naming variables the same as a class name, so I changed it to the plural of the noun "find". I'll rethink the variable name for sure. > >> + if(finds.path.back() != m_struct_path.back()) >> + { >> + /* Same boffset points to members of different classes. >> + We have found an ambiguity and should record it*/ >> + ambiguous = true; >> + } >> + else >> + { >> + /* we don't need to insert this value again, because a >> + non-ambiguous path already leads to it */ > > First letter capital, period + two spaces at the end. > > There are two lines above that have more than 8 spaces, that should be > replaced with a tab. Most editors have a way to configure whitespaces > to do that automatically. > >> + insert = false; > > Since this path makes it so that we'll definitely not insert, we can > break the iteration. Ah, good catch. > > Simon > Thanks for the review! -- Cheers! Bruno Larsen