RE: C++-compat clean build

["Tedeschi, Walfred" <walfred dot tedeschi at intel dot com>]

Ondrej,

I have also seem many GNU style violations in your patch like "=" followed by two spaces.
Parameters not having the right indentation on the next line and so on...

Some of them are here, just in case:

On line 606 it seems that style is off.
Line 734 seems to be too long.
On line 791 you removed a line that does not belong to this patch.
In many places you have  equals followed by two spaces please change for only one, e.g. lines 952 961 970 too many spaces after equal, please verify other places.
Same as before: 1007 1008 1017  1035 1200...1210.

Line 1104 equal is on the next line.
Ax-general.c on the hunk -96,8  -> spaces and tabs are wrong  and 460,8 parameters indentation. 

On i386-tdep.c hunk 7799 parameters are off.
Too long lines more than 80 chars on the hunk linux-nat.c

Remote.c hunks: -587, 8 parameters are off.


Best regards,
-Fred

-----Original Message-----
From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Ondrej Oprala
Sent: Tuesday, October 01, 2013 2:57 PM
To: Jan Kratochvil
Cc: 'gdb-patches@sourceware.org'; Agovic, Sanimir
Subject: Re: C++-compat clean build

On 10/01/2013 02:53 PM, Jan Kratochvil wrote:
> Hi Ondrej,
>
> On Tue, 01 Oct 2013 13:25:34 +0200, Ondrej Oprala wrote:
>> this is the first of a few patches I intend to write to make gdb code 
>> compile cleanly with -Wc++-compat.
>> The idea is to make separate patches for respective subdirs under 
>> gdb/, unless someone objects ofc.
> this is a too huge patch.  It should import first archer/tromey/c++ 
> which is already separated into specific parts, that is each commit in 
> that branch should be a separate posted mail/patch.  This could also 
> state the gcc error that occured, it is not always clear for review (such as the ptrace case).
>
> According to gdb/CONTRIBUTE there should be written ChangeLog entries, 
> that is what will be written to gdb/ChangeLog (one writes them as 
> plain text into the mail, not directly patching the file 
> gdb/ChangeLog, as the ChangeLog patch would get immediately out of 
> scope).  Some requests for comments without immediate check-in may got 
> without ChangeLog entry, such as this preview patch.
>
> It is not a requirement but the preference is to post the patches 
> inlined in the mail text; just I am not sure Thunderbird will not 
> corrupt it, your mail body is format=flowed which would corrupt it, 
> OTOH without format=flowed some mailers wrap the patch to some fixed 
> column.  So maybe the attachment is the least worst for Thunderbird.
>
>
>> --- a/gdb/amd64-linux-nat.c
>> +++ b/gdb/amd64-linux-nat.c
>> @@ -172,7 +172,7 @@ amd64_linux_fetch_inferior_registers (struct target_ops *ops,
>>       {
>>         elf_gregset_t regs;
>>   
>> -      if (ptrace (PTRACE_GETREGS, tid, 0, (long) &regs) < 0)
>> +      if (ptrace ((enum __ptrace_request) PTRACE_GETREGS, tid, 0, 
>> + (long) &regs) < 0)
>>   	perror_with_name (_("Couldn't get registers"));
>>   
>>         amd64_supply_native_gregset (regcache, &regs, -1);
> enum __ptrace_request it is on GNU/Linux but not on other platforms 
> where GDB is compilable.  My guess is the right solution could be:
>
> configure.ac:
> -for gdb_arg1 in 'int' 'long'; do
> +for gdb_arg1 in 'enum __ptrace_request' 'int' 'long'; do
>
>
>> --- a/gdb/amd64-tdep.c
>> +++ b/gdb/amd64-tdep.c
>> @@ -762,12 +762,12 @@ amd64_push_arguments (struct regcache *regcache, int nargs,
>>       AMD64_XMM0_REGNUM + 4, AMD64_XMM0_REGNUM + 5,
>>       AMD64_XMM0_REGNUM + 6, AMD64_XMM0_REGNUM + 7,
>>     };
>> -  struct value **stack_args = alloca (nargs * sizeof (struct value 
>> *));
>> +  struct value **stack_args = (struct value **) alloca (nargs * 
>> + sizeof (struct value *));
> Here the line got longer than 80 columns, this is forbidden by GCS:
> 	https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards
>
> It is not always clear what is best in such case, it may be in some 
> cases for example better to move the initialization from the declaration:
>
>    struct value **stack_args;
>
>    stack_args = (struct value **) alloca (nargs * sizeof (struct value 
> *));
>
>
> Sorry for not reviewing the rest of your patch now, it should be split anyway.
>
>
> Thanks,
> Jan
Thank you for your detailed patch reviews, I'll address the issues.
Thanks,
Ondrej
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052