[PATCH 0/8] drop if around WARN_ON

[PATCH 0/8] drop if around WARN_ON

[Julia Lawall]

These patches convert a conditional with a simple test expression and a
then branch that only calls WARN_ON(1) to just a call to WARN_ON, which
will test the condition.

// <smpl>
@@
expression e;
@@

(
if(<+...e(...)...+>) WARN_ON(1);
|
- if (e) WARN_ON(1);
+ WARN_ON(e);
)// </smpl>

[PATCH 1/8] fs/btrfs: drop if around WARN_ON

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Just use WARN_ON rather than an if containing only WARN_ON(1).

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression e;
@@
- if (e) WARN_ON(1);
+ WARN_ON(e);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 fs/btrfs/backref.c |    3 +--
 fs/btrfs/ctree.c   |    9 +++------
 fs/btrfs/inode.c   |    3 +--
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 208d8aa..a321952 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -890,8 +890,7 @@ again:
 	while (!list_empty(&prefs)) {
 		ref = list_first_entry(&prefs, struct __prelim_ref, list);
 		list_del(&ref->list);
-		if (ref->count < 0)
-			WARN_ON(1);
+		WARN_ON(ref->count < 0);
 		if (ref->count && ref->root_id && ref->parent == 0) {
 			/* no parent == root of tree */
 			ret = ulist_add(roots, ref->root_id, 0, GFP_NOFS);
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index cdfb4c4..8ecb995 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1469,10 +1469,8 @@ int btrfs_realloc_node(struct btrfs_trans_handle *trans,
 	if (cache_only && parent_level != 1)
 		return 0;
 
-	if (trans->transaction != root->fs_info->running_transaction)
-		WARN_ON(1);
-	if (trans->transid != root->fs_info->generation)
-		WARN_ON(1);
+	WARN_ON(trans->transaction != root->fs_info->running_transaction);
+	WARN_ON(trans->transid != root->fs_info->generation);
 
 	parent_nritems = btrfs_header_nritems(parent);
 	blocksize = btrfs_level_size(root, parent_level - 1);
@@ -3403,8 +3401,7 @@ static noinline int __push_leaf_right(struct btrfs_trans_handle *trans,
 	if (push_items == 0)
 		goto out_unlock;
 
-	if (!empty && push_items == left_nritems)
-		WARN_ON(1);
+	WARN_ON(!empty && push_items == left_nritems);
 
 	/* push left to right */
 	right_nritems = btrfs_header_nritems(right);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 95542a1..0d169ab 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1657,8 +1657,7 @@ static noinline int add_pending_csums(struct btrfs_trans_handle *trans,
 int btrfs_set_extent_delalloc(struct inode *inode, u64 start, u64 end,
 			      struct extent_state **cached_state)
 {
-	if ((end & (PAGE_CACHE_SIZE - 1)) == 0)
-		WARN_ON(1);
+	WARN_ON((end & (PAGE_CACHE_SIZE - 1)) == 0);
 	return set_extent_delalloc(&BTRFS_I(inode)->io_tree, start, end,
 				   cached_state, GFP_NOFS);
 }

Re: [PATCH 1/8] fs/btrfs: drop if around WARN_ON

[David Sterba]

On Sat, Nov 03, 2012 at 09:30:18PM +0100, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Just use WARN_ON rather than an if containing only WARN_ON(1).
> 
> A simplified version of the semantic patch that makes this transformation
> is as follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression e;
> @@
> - if (e) WARN_ON(1);
> + WARN_ON(e);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
Reviewed-by: David Sterba <dsterba@suse.cz>

[PATCH 2/8] drivers/scsi/bfa/bfa_svc.c: drop if around WARN_ON

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Just use WARN_ON rather than an if containing only WARN_ON(1).

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression e;
@@
- if (e) WARN_ON(1);
+ WARN_ON(e);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/scsi/bfa/bfa_svc.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/bfa/bfa_svc.c b/drivers/scsi/bfa/bfa_svc.c
index 299c1c8..765b8d0 100644
--- a/drivers/scsi/bfa/bfa_svc.c
+++ b/drivers/scsi/bfa/bfa_svc.c
@@ -626,8 +626,7 @@ bfa_fcxp_init_reqrsp(struct bfa_fcxp_s *fcxp,
 		/*
 		 * alloc required sgpgs
 		 */
-		if (n_sgles > BFI_SGE_INLINE)
-			WARN_ON(1);
+		WARN_ON(n_sgles > BFI_SGE_INLINE);
 	}
 
 }

[PATCH 3/8] arch/x86/kernel/cpu/mtrr/cleanup.c: drop if around WARN_ON

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Just use WARN_ON rather than an if containing only WARN_ON(1).

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression e;
@@
- if (e) WARN_ON(1);
+ WARN_ON(e);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 arch/x86/kernel/cpu/mtrr/cleanup.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/cleanup.c b/arch/x86/kernel/cpu/mtrr/cleanup.c
index 35ffda5..dd833bf 100644
--- a/arch/x86/kernel/cpu/mtrr/cleanup.c
+++ b/arch/x86/kernel/cpu/mtrr/cleanup.c
@@ -967,8 +967,7 @@ int __init mtrr_trim_uncached_memory(unsigned long end_pfn)
 	if (total_trim_size) {
 		pr_warning("WARNING: BIOS bug: CPU MTRRs don't cover all of memory, losing %lluMB of RAM.\n", total_trim_size >> 20);
 
-		if (!changed_by_mtrr_cleanup)
-			WARN_ON(1);
+		WARN_ON(!changed_by_mtrr_cleanup);
 
 		pr_info("update e820 for mtrr\n");
 		update_e820();

[PATCH 4/8] fs/ext3/inode.c: drop if around WARN_ON

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Just use WARN_ON rather than an if containing only WARN_ON(1).

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression e;
@@
- if (e) WARN_ON(1);
+ WARN_ON(e);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 fs/ext3/inode.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
index 7e87e37..b176d42 100644
--- a/fs/ext3/inode.c
+++ b/fs/ext3/inode.c
@@ -1071,8 +1071,7 @@ struct buffer_head *ext3_getblk(handle_t *handle, struct inode *inode,
 	 * mapped. 0 in case of a HOLE.
 	 */
 	if (err > 0) {
-		if (err > 1)
-			WARN_ON(1);
+		WARN_ON(err > 1);
 		err = 0;
 	}
 	*errp = err;

Re: [PATCH 4/8] fs/ext3/inode.c: drop if around WARN_ON

[Jan Kara]

On Sat 03-11-12 21:30:21, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Just use WARN_ON rather than an if containing only WARN_ON(1).
> 
> A simplified version of the semantic patch that makes this transformation
> is as follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression e;
> @@
> - if (e) WARN_ON(1);
> + WARN_ON(e);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
  Thanks. Added into my tree.

							Honza

> ---
>  fs/ext3/inode.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
> index 7e87e37..b176d42 100644
> --- a/fs/ext3/inode.c
> +++ b/fs/ext3/inode.c
> @@ -1071,8 +1071,7 @@ struct buffer_head *ext3_getblk(handle_t *handle, struct inode *inode,
>  	 * mapped. 0 in case of a HOLE.
>  	 */
>  	if (err > 0) {
> -		if (err > 1)
> -			WARN_ON(1);
> +		WARN_ON(err > 1);
>  		err = 0;
>  	}
>  	*errp = err;
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 5/8] drivers/scsi/qla2xxx/qla_nx.c: drop if around WARN_ON

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Just use WARN_ON rather than an if containing only WARN_ON(1).

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression e;
@@
- if (e) WARN_ON(1);
+ WARN_ON(e);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/scsi/qla2xxx/qla_nx.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_nx.c b/drivers/scsi/qla2xxx/qla_nx.c
index f5e297c..4c62a5d 100644
--- a/drivers/scsi/qla2xxx/qla_nx.c
+++ b/drivers/scsi/qla2xxx/qla_nx.c
@@ -388,8 +388,7 @@ qla82xx_pci_set_crbwindow(struct qla_hw_data *ha, u64 off)
 
 	if ((off >= QLA82XX_CRB_PCIX_HOST) && (off < QLA82XX_CRB_PCIX_HOST2)) {
 		/* We are in first CRB window */
-		if (ha->curr_window != 0)
-			WARN_ON(1);
+		WARN_ON(ha->curr_window != 0);
 		return off;
 	}
 

[PATCH 6/8] arch/arm/mach-omap2/dpll3xxx.c: drop if around WARN_ON

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Just use WARN_ON rather than an if containing only WARN_ON(1).

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression e;
@@
- if (e) WARN_ON(1);
+ WARN_ON(e);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 arch/arm/mach-omap2/dpll3xxx.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
index eacf51f..ed855b0 100644
--- a/arch/arm/mach-omap2/dpll3xxx.c
+++ b/arch/arm/mach-omap2/dpll3xxx.c
@@ -478,8 +478,7 @@ int omap3_noncore_dpll_set_rate(struct clk *clk, unsigned long rate)
 		if (!soc_is_am33xx() && !cpu_is_omap44xx() && !cpu_is_omap3630()) {
 			freqsel = _omap3_dpll_compute_freqsel(clk,
 						dd->last_rounded_n);
-			if (!freqsel)
-				WARN_ON(1);
+			WARN_ON(!freqsel);
 		}
 
 		pr_debug("clock: %s: set rate: locking rate to %lu.\n",

Re: [PATCH 6/8] arch/arm/mach-omap2/dpll3xxx.c: drop if around WARN_ON

[Tony Lindgren]

* Julia Lawall <Julia.Lawall@lip6.fr> [121103 13:32]:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Just use WARN_ON rather than an if containing only WARN_ON(1).

Thanks applying this one into omap-for-v3.8/fixes-for-merge-window.

Regards,

Tony
 
> A simplified version of the semantic patch that makes this transformation
> is as follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression e;
> @@
> - if (e) WARN_ON(1);
> + WARN_ON(e);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
>  arch/arm/mach-omap2/dpll3xxx.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
> index eacf51f..ed855b0 100644
> --- a/arch/arm/mach-omap2/dpll3xxx.c
> +++ b/arch/arm/mach-omap2/dpll3xxx.c
> @@ -478,8 +478,7 @@ int omap3_noncore_dpll_set_rate(struct clk *clk, unsigned long rate)
>  		if (!soc_is_am33xx() && !cpu_is_omap44xx() && !cpu_is_omap3630()) {
>  			freqsel = _omap3_dpll_compute_freqsel(clk,
>  						dd->last_rounded_n);
> -			if (!freqsel)
> -				WARN_ON(1);
> +			WARN_ON(!freqsel);
>  		}
>  
>  		pr_debug("clock: %s: set rate: locking rate to %lu.\n",
> 

[PATCH 7/8] drivers/scsi/scsi_transport_fc.c: drop if around WARN_ON

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Just use WARN_ON rather than an if containing only WARN_ON(1).

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression e;
@@
- if (e) WARN_ON(1);
+ WARN_ON(e);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/scsi/scsi_transport_fc.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index e894ca7..f54c945 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -1699,9 +1699,8 @@ fc_stat_show(const struct device *dev, char *buf, unsigned long offset)
 	struct fc_host_statistics *stats;
 	ssize_t ret = -ENOENT;
 
-	if (offset > sizeof(struct fc_host_statistics) ||
-	    offset % sizeof(u64) != 0)
-		WARN_ON(1);
+	WARN_ON(offset > sizeof(struct fc_host_statistics) ||
+		offset % sizeof(u64) != 0);
 
 	if (i->f->get_fc_host_stats) {
 		stats = (i->f->get_fc_host_stats)(shost);

[PATCH 8/8] drivers/net/wireless/ath/ath6kl/hif.c: drop if around WARN_ON

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Just use WARN_ON rather than an if containing only WARN_ON(1).

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression e;
@@
- if (e) WARN_ON(1);
+ WARN_ON(e);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/net/wireless/ath/ath6kl/hif.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/hif.c b/drivers/net/wireless/ath/ath6kl/hif.c
index 68ed6c2..9e47c4a 100644
--- a/drivers/net/wireless/ath/ath6kl/hif.c
+++ b/drivers/net/wireless/ath/ath6kl/hif.c
@@ -338,8 +338,7 @@ static int ath6kl_hif_proc_err_intr(struct ath6kl_device *dev)
 	status = hif_read_write_sync(dev->ar, ERROR_INT_STATUS_ADDRESS,
 				     reg_buf, 4, HIF_WR_SYNC_BYTE_FIX);
 
-	if (status)
-		WARN_ON(1);
+	WARN_ON(status);
 
 	return status;
 }
@@ -383,8 +382,7 @@ static int ath6kl_hif_proc_cpu_intr(struct ath6kl_device *dev)
 	status = hif_read_write_sync(dev->ar, CPU_INT_STATUS_ADDRESS,
 				     reg_buf, 4, HIF_WR_SYNC_BYTE_FIX);
 
-	if (status)
-		WARN_ON(1);
+	WARN_ON(status);
 
 	return status;
 }

Re: [PATCH 8/8] drivers/net/wireless/ath/ath6kl/hif.c: drop if around WARN_ON

[Kalle Valo]

On 11/03/2012 10:30 PM, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Just use WARN_ON rather than an if containing only WARN_ON(1).
> 
> A simplified version of the semantic patch that makes this transformation
> is as follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression e;
> @@
> - if (e) WARN_ON(1);
> + WARN_ON(e);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Thanks, applied to ath6kl.git.

Kalle

Re: [PATCH 0/8] drop if around WARN_ON

[Sasha Levin]

Hi Julia,

On Sat, Nov 3, 2012 at 4:30 PM, Julia Lawall <Julia.Lawall@lip6.fr> wrote:
> These patches convert a conditional with a simple test expression and a
> then branch that only calls WARN_ON(1) to just a call to WARN_ON, which
> will test the condition.
>
> // <smpl>
> @@
> expression e;
> @@
>
> (
> if(<+...e(...)...+>) WARN_ON(1);
> |
> - if (e) WARN_ON(1);
> + WARN_ON(e);
> )// </smpl>

So this deals with WARN_ON(), are you considering doing the same for
the rest of it's friends?


Thanks,
Sasha

Re: [PATCH 0/8] drop if around WARN_ON

[Julia Lawall]

On Sun, 4 Nov 2012, Sasha Levin wrote:

> Hi Julia,
>
> On Sat, Nov 3, 2012 at 4:30 PM, Julia Lawall <Julia.Lawall@lip6.fr> wrote:
>> These patches convert a conditional with a simple test expression and a
>> then branch that only calls WARN_ON(1) to just a call to WARN_ON, which
>> will test the condition.
>>
>> // <smpl>
>> @@
>> expression e;
>> @@
>>
>> (
>> if(<+...e(...)...+>) WARN_ON(1);
>> |
>> - if (e) WARN_ON(1);
>> + WARN_ON(e);
>> )// </smpl>
>
> So this deals with WARN_ON(), are you considering doing the same for
> the rest of it's friends?

I tried WARN_ON_ONCE, but the pattern never occurred.  Are there others 
that are worth trying?

thanks,
julia

Re: [PATCH 0/8] drop if around WARN_ON

[Sasha Levin]

On Sun, Nov 4, 2012 at 10:57 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> On Sun, 4 Nov 2012, Sasha Levin wrote:
>
>> Hi Julia,
>>
>> On Sat, Nov 3, 2012 at 4:30 PM, Julia Lawall <Julia.Lawall@lip6.fr> wrote:
>>>
>>> These patches convert a conditional with a simple test expression and a
>>> then branch that only calls WARN_ON(1) to just a call to WARN_ON, which
>>> will test the condition.
>>>
>>> // <smpl>
>>> @@
>>> expression e;
>>> @@
>>>
>>> (
>>> if(<+...e(...)...+>) WARN_ON(1);
>>> |
>>> - if (e) WARN_ON(1);
>>> + WARN_ON(e);
>>> )// </smpl>
>>
>>
>> So this deals with WARN_ON(), are you considering doing the same for
>> the rest of it's friends?
>
>
> I tried WARN_ON_ONCE, but the pattern never occurred.  Are there others that
> are worth trying?

Definitely!

Here's the semantic patch I've got:

@@
expression e;
@@

(
- if (e) WARN_ON(1);
+ WARN_ON(e);
|
- if (e) WARN_ON_ONCE(1);
+ WARN_ON_ONCE(e);
|
- if (e) WARN_ON_SMP(1);
+ WARN_ON_SMP(e);
|
- if (e) BUG();
+ BUG_ON(e);
)

This gave me a really huge patch output.

I can send it out if you think the patch above looks good.


Thanks,
Sasha

Re: [PATCH 0/8] drop if around WARN_ON

[Julia Lawall]

On Sun, 4 Nov 2012, Sasha Levin wrote:

> On Sun, Nov 4, 2012 at 10:57 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> On Sun, 4 Nov 2012, Sasha Levin wrote:
>>
>>> Hi Julia,
>>>
>>> On Sat, Nov 3, 2012 at 4:30 PM, Julia Lawall <Julia.Lawall@lip6.fr> wrote:
>>>>
>>>> These patches convert a conditional with a simple test expression and a
>>>> then branch that only calls WARN_ON(1) to just a call to WARN_ON, which
>>>> will test the condition.
>>>>
>>>> // <smpl>
>>>> @@
>>>> expression e;
>>>> @@
>>>>
>>>> (
>>>> if(<+...e(...)...+>) WARN_ON(1);
>>>> |
>>>> - if (e) WARN_ON(1);
>>>> + WARN_ON(e);
>>>> )// </smpl>
>>>
>>>
>>> So this deals with WARN_ON(), are you considering doing the same for
>>> the rest of it's friends?
>>
>>
>> I tried WARN_ON_ONCE, but the pattern never occurred.  Are there others that
>> are worth trying?
>
> Definitely!
>
> Here's the semantic patch I've got:
>
> @@
> expression e;
> @@
>
> (
> - if (e) WARN_ON(1);
> + WARN_ON(e);
> |
> - if (e) WARN_ON_ONCE(1);
> + WARN_ON_ONCE(e);
> |
> - if (e) WARN_ON_SMP(1);
> + WARN_ON_SMP(e);
> |
> - if (e) BUG();
> + BUG_ON(e);
> )
>
> This gave me a really huge patch output.
>
> I can send it out if you think the patch above looks good.

I didn't change any cases where the if test contains a function call.  The 
current definitions of WARN_ON seem to always evaluate the condition 
expression, but I was worried that that might not always be the case.  And 
calling a function (the ones I remember were some kinds of print 
functions) seems like something one might not want buried in the argument 
of a debugging macro.

WARN_ON_SMP is just WARN_ON if CONFIG_SMP is true, but it is just 0 
otherwise.  So in that case it seems important to check that one is not 
throwing away something important.

I remember working on the BUG_ON case several years ago, and other people 
worked on it too, but I guess some are still there...  The current 
definitions of BUG_ON seem to keep the condition, but there are quite a 
few specialized definitions, so someone at some point might make a version 
that does not have that property.

julia

Re: [PATCH 0/8] drop if around WARN_ON

[Sasha Levin]

On Sun, Nov 4, 2012 at 11:16 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> I didn't change any cases where the if test contains a function call.  The
> current definitions of WARN_ON seem to always evaluate the condition
> expression, but I was worried that that might not always be the case.  And
> calling a function (the ones I remember were some kinds of print functions)
> seems like something one might not want buried in the argument of a
> debugging macro.

Makes sense.

> WARN_ON_SMP is just WARN_ON if CONFIG_SMP is true, but it is just 0
> otherwise.  So in that case it seems important to check that one is not
> throwing away something important.

Yup, we just need to make sure that the expression being evaluated doesn't
have side-effects.

> I remember working on the BUG_ON case several years ago, and other people
> worked on it too, but I guess some are still there...  The current
> definitions of BUG_ON seem to keep the condition, but there are quite a few
> specialized definitions, so someone at some point might make a version that
> does not have that property.

It makes sense to keep an eye for such things when converting code. I
also don't think we'll get to see a version of BUG_ON which doesn't
evaluate the expression since the kernel already has more than enough
BUG_ONs that assume the expression will be evaluated:

BUG_ON(HYPERVISOR_callback_op(CALLBACKOP_register, &event));
BUG_ON(gpiochip_add(&gemini_gpio_chip));
BUG_ON(clocksource_register_hz(&sirfsoc_clocksource, CLOCK_TICK_RATE));
BUG_ON(gpio_request(ZOOM2_HEADSET_MUX_GPIO, "hs_mux") < 0);

And so on, so we're probably safe converting to BUG_ON even if the
condition is a function, as long as it doesn't create a long and
complicated BUG_ON() ofcourse.


Thanks,
Sasha