Re: [PATCH 0/5] Remove a few hurdles of compiling with clang

[Simon Marchi <simon.marchi@ericsson.com>]

On 2017-06-13 11:14 AM, Yao Qi wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>> That's precisely the reason why I raised this: it would be good to
>> have a clear policy on this, to avoid the need for unnecessary work
>> and unnecessary disputes.  (I actually hoped we already did have such
>> a policy, but if not, I think we should try to come up with it.)
> 
> In general, it is good to keep GDB built by different popular compilers,
> so people are easy to build GDB and different warnings from different
> compilers will catch more bugs in GDB.  On the other hand, GCC is still
> the primary compiler to build GDB, and support of other compilers in
> building GDB should not undermine the case that GDB is built by GCC.
> For example, it is not acceptable to build GDB with compiler X but break
> the build with GCC.  We still must fix the GDB build failure with GCC, as
> what we did in the past, and we welcome the contributions to fix the GDB
> build with other compilers.
> 

I think that makes sense.

If somebody is willing to do the work and that it doesn't degrade the code quality,
we should have no problem accepting it.  So if it's a "side-step" that allows both
compilers to be happy, that's ok.  As a patch submitter, if you use primarily GCC,
you are not required to test your patches with Clang, but if you use primarily Clang,
you must test your patch with GCC (a version that's easily accessible for you).

Does that sound like a good rule?

I have a concrete example that is currently in the pipeline.  I hit this warning/error:

/home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:209:7: error: variable 'i' is incremented both in the loop header and in the loop body [-Werror,-Wfor-loop-analysis]
      i++;
      ^
/home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:199:32: note: incremented here
  ALL_DEBUG_ADDRESS_REGISTERS (i)
                               ^

which would require changing this code:

  ALL_DEBUG_ADDRESS_REGISTERS (i)
    {
      ...
      i++;
    }

to either the expansion of the macro:

  for (int i = DR_FIRSTADDR; i <= DR_LASTADDR; i += 2)
    {
      ...
    }

or to a new macro that would take into account the increment:

  ITER_DEBUG_ADDRESS_REGISTERS (i, i += 2)  // other users would use i++
    {
      ...
    }

or something else.

I don't think it makes the code worst.  One could even argue that the current code
breaks the "abstraction" of the macro anyway, so it's not ideal.  But overall, I think
that eliminating the error like this is better than adding -Wno-for-loop-analysis, because
if I wrote code like this and it was actually a mistake, I would like the compiler to tell
me.

Simon