Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 322119

Summary: [checker] Suspicious semicolon (i.e. ";" after "if")
Product: [Tools] CDT Reporter: Kirstin Weber <kirstin.weber>
Component: cdt-codanAssignee: Elena Laskavaia <elaskavaia.cdt>
Status: RESOLVED FIXED QA Contact: Elena Laskavaia <elaskavaia.cdt>
Severity: enhancement    
Priority: P3 CC: cdtdoug, fchen0000, jens.elmenthaler, malaperle, recoskie
Version: 8.0   
Target Milestone: 8.0   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
Suspicious semicolon checker elaskavaia.cdt: iplog+

Description Kirstin Weber CLA 2010-08-09 06:49:49 EDT 
Build Identifier: 

I had the problem that I wrote a ";" after an "if" condition and could not find the problem for a long time. 

Example: 

if(myVariable == 1);  // <= here I accidently added a ";"
{
  // do something
}

It would be helpful if a checker would check for this case and show a hint that there might be a problem. 

Reproducible: Always

Steps to Reproduce:
See details
Comment 1 Marc-André Laperle CLA 2010-09-17 12:48:40 EDT 
Should it only check for 

if(expr);
{
 foo();
}

or also

if(expr);
foo();
Comment 2 Andrew Gvozdev CLA 2010-09-17 14:15:26 EDT 
I think it should check second case too. Consider
if(expr);
   foo(); // indented line here

If semicolon is intended (i.e. "expr" produces side effects) one could be more explicit:
if(expr){}
Comment 3 Kirstin Weber CLA 2010-09-20 03:55:03 EDT 
I agree with Andrew. I think in most cases the semicolon after "if(expr);" is not intended.
Comment 4 Marc-André Laperle CLA 2010-09-22 13:08:53 EDT 
My first approach was comparing the line number of the condition and the line number of the null statement in AST. But the condition doesn't include the parentheses which means this will not report an error but should:

if(cond
   );

I'm trying to figure out a way to get the line number of the parenthesis at the end of the condition in a cheap manner. And getTrailingSyntax doesn't include white space characters, humm.
Comment 5 Andrew Gvozdev CLA 2010-09-22 15:55:54 EDT 
It occurred to me that there is a legitimate case with semicolon:

if(expr);
else
   foo();
Comment 6 Chris Recoskie CLA 2010-09-22 16:06:17 EDT 
(In reply to comment #5)
> It occurred to me that there is a legitimate case with semicolon:
> if(expr);
> else
>    foo();

It's legit, but not entirely sure it's desirable either.  Isn't that more succinctly/elegantly expressed as:

if(!expr)
   foo();

It's a matter of taste I suppose.
Comment 7 Elena Laskavaia CLA 2010-09-29 21:43:18 EDT 
Not sure why you are trying do deal with line numbers, just check what is the statement on the if branch - if it empty statement - it is "if (a);" case

(In reply to comment #4)
> My first approach was comparing the line number of the condition and the line
> number of the null statement in AST. But the condition doesn't include the
> parentheses which means this will not report an error but should:
> 
> if(cond
>    );
> 
> I'm trying to figure out a way to get the line number of the parenthesis at the
> end of the condition in a cheap manner. And getTrailingSyntax doesn't include
> white space characters, humm.
Comment 8 Marc-André Laperle CLA 2010-09-29 22:21:04 EDT 
(In reply to comment #7)
> Not sure why you are trying do deal with line numbers, just check what is the
> statement on the if branch - if it empty statement - it is "if (a);" case

It depends. Should it consider this case suspicious or not:

if(a)
  ;
Comment 9 Elena Laskavaia CLA 2010-09-30 11:53:35 EDT 
yes. Why do you write a code like that?
Comment 10 Marc-André Laperle CLA 2010-10-01 12:49:41 EDT 
Created attachment 180073 [details]
Suspicious semicolon checker
Comment 11 Elena Laskavaia CLA 2010-10-17 21:04:19 EDT 
assigned
Comment 12 Elena Laskavaia CLA 2010-10-17 21:19:29 EDT 
Thanks! patch applied. I added a bit of code to protect from case where empty macro is used as statement
Comment 14 Kirstin Weber CLA 2010-10-27 04:09:51 EDT 
Thanks for the checker!
Comment 15 Feng Chen CLA 2021-03-11 23:55:55 EST 
(In reply to Chris Recoskie from comment #6)
> (In reply to comment #5)
> > It occurred to me that there is a legitimate case with semicolon:
> > if(expr);
> > else
> >    foo();
> 
> It's legit, but not entirely sure it's desirable either.  Isn't that more
> succinctly/elegantly expressed as:
> 
> if(!expr)
>    foo();
> 
> It's a matter of taste I suppose.

I had one occasion the logic is much more clear when writing like:

if(expr);
else if(expr2) do_something;
else if(expr3) do_sth_else;
else do_sth;