Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

[Qu Wenruo <quwenruo@cn.fujitsu.com>]

At 12/06/2016 08:44 PM, Goldwyn Rodrigues wrote:
>
>
> On 12/05/2016 09:08 PM, Qu Wenruo wrote:
>>
>>
>> At 12/06/2016 10:51 AM, Goldwyn Rodrigues wrote:
>>>
>>>
>>> On 12/05/2016 08:03 PM, Qu Wenruo wrote:
>>>> BTW, the DISABLE_BACKTRACE branch seems quite different from
>>>> backtrace one.
>>>>
>>>> #define BUG_ON(c) assert_trace(#c, __FILE__, __func__, __LINE__,
>>>> (long)(c))
>>>> #define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__,
>>>> (long)(c))
>>>> #define ASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__,
>>>> (long)!(c))
>>>> #define BUG() assert_trace(NULL, __FILE__, __func__, __LINE__, 1)
>>>> #else
>>>> #define BUG_ON(c) assert(!(c))
>>>> #define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__,
>>>> (long)(c))
>>>> #define ASSERT(c) assert(!(c))
>>>> #define BUG() assert(0)
>>>>
>>>> Condition of BUG_ON/ASSERT/BUG are all logical notted for
>>>> DISABLE_BACKTRACE.
>>>> While WARN_ON() of both branch are the same condition.
>>>
>>> WARN_ON is using warning_trace as opposed to assert, and that is the
>>> reason it is not notted.
>>>
>>>>
>>>> This seems quite confusing to me.
>>>>
>>>> Any idea to make it more straightforward?
>>>>
>>>
>>> I just kept it the same as before. warning_trace was using an extra
>>> variable, trace, which was not needed because the print_trace was
>>> already in ifndefs.
>>
>> I mean, better make the condition the same for both BUG/BUG_ON/ASSERT.
>> So that we don't need to manually logical not the condition.
>
> First of all, ASSERT and BUG_ON have opposite meanings. ASSERT() checks
> if the condition is true and continues (halts if false). BUG_ON() "bugs"
> if condition is true and halts (continues if false). So you would have
> to use opposite conditions.

I know, I mean, for both backtrace disabled and enabled case, the 
condition should be the same.

If not the same condition, it means assert_trace() has different meaning 
than original assert.

>
>>
>> For example:
>> #define ASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__,（long）
>> (c))
>> and
>> #define ASSERT(c) assert((c))
>>
>> This looks much more straightforward, and easier to expose bug at review
>> time.
>
> Could you explain with a patch? You idea seems to add more code than
> reduce it.

Sure, will submit one soon.

Thanks,
Qu