Issues handling macros that don't require semicolons

[stbrowne]

I added test cases that catch both of these things. However, the original minimal repro causes a false positive uninitMemberVar:

struct InitTest
{
	InitTest() { Init(); }
private:
	SOME_MACRO_WITH_NO_SEMICOLON()
	void Init() { m_var = 0; }
	int m_var;
};

[PKEuS]

The tab parsing test looks like it should be moved to testtokenlist or testtokenize. It does not seem to be symboldatabase related.

[stbrowne]

You're right. I thought I looked for a better place, but clearly I didn't look hard enough. Sorry, that's just my lack of knowledge of the code base. I've moved the test to testtokenlist.cpp instead and modified it accordingly.

[danmar]

Thanks for these fixes!!

I don't believe tokenlist needs to handle the tabs. The Preprocessor does not output tabs as far as I know. Could you show some source file that cause some problem?

Macros without semicolon: in such cases we try to tell the user to configure cppcheck better. The function name would probably be "Init" in such code:

void Init() NORETURN THROW(std::exception) {}

The severity should be "information". Not sure if you have to use --check-config to see such messages. Feel free to investigate..

[stbrowne]

I don't know how you could really configure cppcheck to avoid this issue caused by macros with no semicolons. Even using the Init function declaration you suggest you still get the same warning:

cppcheck.exe macro.cpp --enable=warning
Checking macro.cpp ...
[macro.cpp:3]: (warning) Member variable 'InitTest::m_var' is not initialized in the constructor.

I think you might be assuming the macro is in some way related to the function. However, in these cases they are not. It's wxWidgets related macros that just happen to be above the line in question without a semicolon. For instance:

class MyWindow : public wxWindow
{
public:
    // Insert code here
private:
    DECLARE_DYNAMIC_CLASS(MyWindow)

    void Init();
};

I don't want to disable warnings or uninitMemberVar because we want to catch those. These are just false positives caused by the fact that the macro immediately preceding the Init function causes Init to not make it into the symbolDatabase functionList. So any check that needs to follow Init's logic would be broken.

Your code does demonstrate another interesting issue though. My fix would not handle non-macro based pre-processor defines (ie no parenthesis required either) being used immediately before a function call.

#define DECLARE_INT int m_int;
struct InitTest
{
    InitTest() { Init(); }
private:
    DECLARE_INT
    void Init() NORETURN THROW(std::exception) { m_var = 0; }
    int m_var;
};

I can't seem to demonstrate any actual errors caused by the tab parsing from the command line for some reason. However, my original pull request had a unit test in symbol database demonstrating how it caused the function to not show up in the function list if the member function had a tab preceding it because the previous token would be "\tvoid" which wouldn't match what it wanted. If you change the spaces to tabs in the macroFunctionTests I added and revert the tab parsing change then they will fail which is what lead me to fix it.

[stbrowne]

I see what you mean regarding tabs and the preprocessor now. It makes sense that the unit tests wouldn't involve the preprocessor unless that's what's being tested. I see that's being done in Preprocessor::preprocessWhitespaces. I can remove the commit for the tab changes and just leave the macros tests and fix.

[danmar]

It's wxWidgets related macros that just happen to be above the line in question without a semicolon.

ah nice... that configuration could be updated a lot.

open the cfg/wxwidgets.cfg

add this:

<define name="DECLARE_DYNAMIC_CLASS(C)" value=""/>

Then if you run cppcheck with --library=wxwidgets , you should get proper warnings:

cppcheck --library=wxwidgets.cfg ....

There is a lot you could configure to get better wxwidgets checking! I hope that sounds interesting. I would love to push some improvements in that file.

[danmar]

Example:

daniel@debian:~/cppcheck$ cat 898.cpp 
struct InitTest
{
    InitTest() { Init(); }
private:
    DECLARE_DYNAMIC_CLASS(InitTest)
    void Init() { m_var = 0; }
    int m_var;
};

daniel@debian:~/cppcheck$ ./cppcheck --enable=warning 898.cpp 
Checking 898.cpp ...
[898.cpp:3]: (warning) Member variable 'InitTest::m_var' is not initialized in the constructor.

daniel@debian:~/cppcheck$ ./cppcheck --library=wxwidgets --enable=warning 898.cpp 
Checking 898.cpp ...

[danmar]

I want to have a cfg fix. I am curious if my wxwidgets.cfg fix worked.

It's not possible to avoid user configuration here.

With your fix the SymbolDatabase will make better guesses in some cases and wrong guesses in other cases.

[stbrowne]

I really wish there was a better way to handle this because it may mask other issues that I don't know about. I covered the most common macros, but I don't the time to search our code base for all possible macros that could possibly be causing issues. I simply took our cpp.hint file which we use to fix similar issues with macros and Visual Studio intellisense which I just recently discovered and added the same defines.

Here is the end result:

  <!-- Override macros which don't necessarily require semicolons at the end -->
  <define name="DECLARE_CLASS(n)" value= ""/>
  <define name="DECLARE_DYNAMIC_CLASS(n)" value= ""/>
  <define name="DECLARE_ABSTRACT_CLASS(n)" value= ""/>
  <define name="IMPLEMENT_DYNAMIC_CLASS(n,b)" value= ""/>
  <define name="IMPLEMENT_ABSTRACT_CLASS(n,b)" value= ""/>
  <define name="DECLARE_EVENT_TYPE(name, value)" value= ""/>
  <define name="DECLARE_LOCAL_EVENT_TYPE(name, value)" value= ""/>
  <define name="DEFINE_EVENT_TYPE(name)" value= ""/>
  <define name="DEFINE_LOCAL_EVENT_TYPE(name)" value= ""/>
  <define name="DECLARE_EVENT_TABLE()" value= ""/>
  <define name="wxDECLARE_EVENT( name, type )" value= ""/>
  <define name="wxDECLARE_NO_COPY_CLASS(classname)" value= ""/>
  <define name="wxDEFINE_EVENT( name, type )" value= ""/>

  <!-- Override begin/end macros slightly differently -->
  <!-- In order to avoid overriding all the possible event types we just replace with a semicolon -->
  <!-- We could turn these into multi-line comments, but that wouldn't work if there was one inside the begin/end -->
  <define name="BEGIN_EVENT_TABLE(a,b)" value= ";"/>
  <define name="END_EVENT_TABLE()" value= ";"/>
  <define name="wxBEGIN_EVENT_TABLE(a,b)" value= ";"/>
  <define name="wxEND_EVENT_TABLE()" value= ";"/>

Note that inside the BEGIN/END macros there are several EVT_* macros for various different types of event handlers. There are way too many different event types for me to define them all. So if I were just to define END_EVENT_TABLE to be replaced by an empty string then the previous EVT_* macro which doesn't end with a semicolon would then cause the same problem so that's why they're replaced with semicolons.

[danmar]

I really wish there was a better way to handle this because it may mask other issues that I don't know about.

My solution is not "written in stone". I think it's okay to try to come up with a better solution/configuration.

I covered the most common macros, but I don't the time to search our code base for all possible macros that could possibly be causing issues.

I think that's an okay approach. Take care of the macros that cause problems for you. This can be extended by others.

[danmar]

I applied your macros with 26ec339. Hope the analysis of your code works better now.