Re: [PATCH] Update ENABLE_CHECKING to make it usable in "if" conditions

[Mikhail Maltsev <maltsevm at gmail dot com>]

On 09/10/2015 12:07 AM, Jeff Law wrote:
>> On Mon, Aug 31, 2015 at 7:49 AM, Mikhail Maltsev <maltsevm@gmail.com>
>>> In lra.c we have:
>>>
>>> #ifdef ENABLE_CHECKING
>>>
>>>   /* Function checks RTL for correctness. If FINAL_P is true, it is
>>>      done at the end of LRA and the check is more rigorous.  */
>>>   static void
>>>   check_rtl (bool final_p)
>>> ...
>>> #ifdef ENABLED_CHECKING
>>>              extract_constrain_insn (insn);
>>> #endif
>>> ...
>>> #endif /* #ifdef ENABLE_CHECKING */
>>>
>>> The comment for extract_constrain_insn says:
>>> /* Do uncached extract_insn, constrain_operands and complain about
>>> failures.
>>>     This should be used when extracting a pre-existing constrained
>>> instruction
>>>     if the caller wants to know which alternative was chosen.  */
>>>
>>> So, as I understand, this additional check can be useful. This patch
>>> removes
>>> "#ifdef ENABLED_CHECKING" (without regressions).
> No strong opinions here.  There's other things that would catch this
> later.  The check merely catches it closer to the point where things go
> wrong.  So I'd tend to want it to be conditional.
> 
But please notice that the inner condition checks for
"ENABLE*D*_CHECKING", so the code guarded by it does not get executed
even in "checking" build. Such spelling is not used anywhere in the code
(except for this place in lra.c), so for me it looks like a typo (if it
is not, then it's really misleading, because it's very easy to confuse
ENABLE/ENABLED_CHECKING). Changing the condition to "ENABLE_CHECKING"
will make it useless because the code is already guarded by
ENABLE_CHECKING, that's why I proposed to remove the "#ifdef".

On 09/16/2015 10:10 PM, Jeff Law wrote:
>> I'd like to see it as part of the patch, even if the initial
>> implementation would be a
>>
>> static const int flag_checking = CHECKING_P;
>>
>> in flags.h so we can avoid chaging all uses twice.  It requires some
>> more careful
>> review of what uses are changed to flag_checking and which one to
>> CHECKING_P
>> of course which means it would make sense to do it as a pre patch
>> handling the
>> obvious cases first.  And then adding -fchecking "properly" wouldn't
>> be too hard
>> anyway.
> That works for me.  Mikhail, can you move things in that direction?

Yes, I hope to continue working on this patch (i.e. split it into
smaller pieces and convert it to use flags instead of macros) during
this weekend.

-- 
Regards,
    Mikhail Maltsev