1. Summary
  2. Files
  3. Support
  4. Report Spam
  5. Create account
  6. Log in

Ticket #3385 (closed defect: fixed)

Opened 2 years ago

Last modified 2 years ago

Tokenizer: 'throw' isn't removed if the argument of the function is type 'struct|class'

Reported by: edward-san Owned by: edward-san
Priority: Normal Milestone: 1.55
Component: Improve check Keywords: patch
Cc:

Description

Minimal example from ticket #3381:

void foo (struct S & Val) throw ();
./cppcheck progtest/test.cpp --debug
Checking progtest/test.cpp...


##file progtest/test.cpp
1: void foo ( struct S & Val@1 ) throw ( ) ;

Attachments

ticket3385.diff (2.0 KB) - added by edward-san 2 years ago.
proposed patch

Change History

Changed 2 years ago by edward-san

proposed patch

Changed 2 years ago by edward-san

  • keywords patch added
  • owner changed from noone to edward-san
  • status changed from new to accepted

Attached a patch which fixes this problem.

Changed 2 years ago by robertreif

Why are you processing a function parameter as a class definition?

Changed 2 years ago by edward-san

What do you mean? Sorry I don't get it.

Changed 2 years ago by robertreif

What is the purpose of this line?

else if (Token::Match(tok, "class|namespace|struct %type%")) {

Changed 2 years ago by edward-san

... to re-call 'removeExceptionSpecifications' when there's a scope after a declaration of class|namespace|struct ?

Changed 2 years ago by edward-san

If I don't add ',' and ')' inside 'while', then it will terminate after the 'throw()', hence 'throw' is ignored.

Changed 2 years ago by edward-san

Or should I declare 'tok1' which does the hard work instead of 'tok'?

Changed 2 years ago by robertreif

The purpose of that line is to find the start of a non-executable scope (i.e. class definition). Adding a ":|{" to the pattern will fix that problem.

The pattern also doesn't handle anonymous namespaces. That should also be fixed.

The whole check doesn't handle function local classes and structures. That requires also tracking and checking function scopes rather than skipping them.

Changed 2 years ago by edward-san

Then it's better IMHO to rewrite this function in order to make this function non-recursive. I'll see what I can do.

Changed 2 years ago by edward-san

As a temporary workaround (with robertreif suggestion), I've committed this:
https://github.com/danmar/cppcheck/commit/eb5fe250abf144bd67314b16c324b15b9914ef57

The first two things are done, the third one will be fixed later.

Changed 2 years ago by robertreif

Anonymous structures and unions should also be processed.

Changed 2 years ago by edward-san

Done in commit
https://github.com/danmar/cppcheck/commit/149ff355e230c7e4d3d59fcf9fd331d3d48d5ce6

Can you add some test cases for anonymous structures and unions? What about also some todo test cases related to local variables?

Changed 2 years ago by thevbm

  • milestone changed from 1.53 to 1.54

Changed 2 years ago by pkeus

I wonder why we use that complex way to remove the specifications, with recursion and checking for several types of scopes.
I tried this implementation:

void Tokenizer::removeExceptionSpecifications(Token *tok) const
{
    while (tok) {
        if (Token::Match(tok, ") const| throw (")) {
            if (tok->next()->str() == "const") {
                Token::eraseTokens(tok->next(), tok->linkAt(3));
                tok = tok->next();
            } else
                Token::eraseTokens(tok, tok->linkAt(2));
            tok->deleteNext();
        }

        tok = tok->next();
    }
}

It is 10 times faster, 23 lines shorter and produces no test failures.
We currently jump over executable scopes because we assume that they couldn't contain exception specifications, but that is imho a problem, since you can define classes inside them. And inside such a class, exeption specifications are allowed.

Changed 2 years ago by pkeus

  • milestone changed from 1.54 to 1.55

If there are no objections, I would like to commit that code in a few days.

Changed 2 years ago by pkeus

  • status changed from accepted to closed
  • resolution set to fixed
Note: See TracTickets for help on using tickets.