Re: [PATCH] PR gdb/28480: Improve ambiguous member detection

[Bruno Larsen <blarsen@redhat.com>]

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