Developer forums (C::B DEVELOPMENT STRICTLY!) > CodeCompletion redesign
Macro expansion infinite loop.
blauzahn:
Although my humble contribution is not directly related to the infinite loop, it may help the codecompletion a bit.
If I run cppcheck I get a bunch of remarks:
--- Code: ---$ cppcheck --enable=all *.cpp |grep -v Checking |grep -v 'files checked'
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_Parent' is not assigned a value in 'CCLogger::operator='.
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_LogId' is not assigned a value in 'CCLogger::operator='.
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_DebugLogId' is not assigned a value in 'CCLogger::operator='.
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_AddTokenId' is not assigned a value in 'CCLogger::operator='.
[tokenizer.h:249]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[expression.cpp:125]: (style) Expression is always false because 'else if' condition matches previous condition at line 104.
[parser.cpp:782] -> [parser.cpp:786]: (performance) Variable 'isParsed' is reassigned a value before the old one has been used.
[parser_base.cpp:180]: (warning) Member variable 'ParserBase::m_BrowserOptions' is not initialized in the constructor.
[parserthread.cpp:1883] -> [parserthread.cpp:1884]: (performance) Variable 'current' is reassigned a value before the old one has been used.
[parserthread.cpp:2819]: (performance) Possible inefficient checking for 'components' emptiness.
[parserthread.cpp:2822]: (performance) Possible inefficient checking for 'components' emptiness.
[parserthread.cpp:3500]: (performance) Possible inefficient checking for 'results' emptiness.
[searchtree.cpp:598]: (style) The scope of the variable 'nchild' can be reduced.
[searchtree.cpp:670]: (style) The scope of the variable 'n' can be reduced.
[searchtree.cpp:759]: (style) The scope of the variable 'matches' can be reduced.
[searchtree.cpp:989]: (style) The scope of the variable 'u' can be reduced.
[searchtree.cpp:77]: (performance) Possible inefficient checking for 'm_Children' emptiness.
[searchtree.cpp:403]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:411]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:460]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:471]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:484]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:510]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[token.cpp:238]: (performance) Possible inefficient checking for 'files' emptiness.
[token.cpp:295]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[tokentree.cpp:150]: (style) The scope of the variable 'result' can be reduced.
[tokentree.cpp:178]: (style) The scope of the variable 'result' can be reduced.
[tokentree.cpp:210]: (style) The scope of the variable 'result' can be reduced.
[tokentree.cpp:243]: (style) The scope of the variable 'result' can be reduced.
[cctreectrl.cpp:86]: (style) C-style pointer casting
[cctreectrl.cpp:166]: (style) C-style pointer casting
[cctreectrl.cpp:167]: (style) C-style pointer casting
[classbrowser.cpp:286]: (style) C-style pointer casting
[classbrowser.cpp:477]: (style) C-style pointer casting
[classbrowser.cpp:492]: (style) C-style pointer casting
[classbrowser.cpp:539]: (style) C-style pointer casting
[classbrowser.cpp:544]: (style) C-style pointer casting
[classbrowser.cpp:807]: (style) C-style pointer casting
[classbrowserbuilderthread.cpp:66]: (warning) Member variable 'ClassBrowserBuilderThread::m_idThreadEvent' is not initialized in the constructor.
[codecompletion.cpp:1964]: (performance) Possible inefficient checking for 'result' emptiness.
[codecompletion.cpp:1749]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[coderefactoring.cpp:438]: (style) The scope of the variable 'pos' can be reduced.
[doxygen_parser.cpp:845]: (performance) Possible inefficient checking for 'tokensIdx' emptiness.
[doxygen_parser.cpp:964]: (performance) Possible inefficient checking for 'result' emptiness.
[nativeparser_base.cpp:1217] -> [nativeparser_base.cpp:1219]: (warning) Possible null pointer dereference: token - otherwise it is redundant to check it against null.
[nativeparser_base.cpp:1247] -> [nativeparser_base.cpp:1249]: (warning) Possible null pointer dereference: token - otherwise it is redundant to check it against null.
[nativeparser_base.cpp:869]: (style) Unused variable: autualTypeResult
[nativeparser_base.cpp:141]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[nativeparser_base.cpp:167]: (performance) Prefer prefix ++/-- operators for non-primitive types.
--- End code ---
IMHO, most of them are little obvious things can be corrected with little effort. This might save ollydbg and huki time for the harder things.
Hope, I do not upset somebody in hijacking the thread to some extent. Thought it might fit here and was not worth opening a new one.
Thanks.
ollydbg:
Hello, blauzahn, thank you very much!
I fixed most of them in the trunk, see below:
--- Code: ---[cclogger.h:49]: (warning) Member variable 'CCLogger::m_Parent' is not assigned a value in 'CCLogger::operator='.
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_LogId' is not assigned a value in 'CCLogger::operator='.
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_DebugLogId' is not assigned a value in 'CCLogger::operator='.
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_AddTokenId' is not assigned a value in 'CCLogger::operator='.
Not sure what is a correct fix.
[tokenizer.h:249]: (performance) Prefer prefix ++/-- operators for non-primitive types.
No need to fix
[expression.cpp:125]: (style) Expression is always false because 'else if' condition matches previous condition at line 104.
fixed
[parser.cpp:782] -> [parser.cpp:786]: (performance) Variable 'isParsed' is reassigned a value before the old one has been used.
no need to fix
[parser_base.cpp:180]: (warning) Member variable 'ParserBase::m_BrowserOptions' is not initialized in the constructor.
fixed
[parserthread.cpp:1883] -> [parserthread.cpp:1884]: (performance) Variable 'current' is reassigned a value before the old one has been used.
fixed.
[parserthread.cpp:2819]: (performance) Possible inefficient checking for 'components' emptiness.
[parserthread.cpp:2822]: (performance) Possible inefficient checking for 'components' emptiness.
no need to fix
[parserthread.cpp:3500]: (performance) Possible inefficient checking for 'results' emptiness.
no need to fix
[searchtree.cpp:598]: (style) The scope of the variable 'nchild' can be reduced.
fixed
[searchtree.cpp:670]: (style) The scope of the variable 'n' can be reduced.
fixed, but maybe n get reassigned?
[searchtree.cpp:759]: (style) The scope of the variable 'matches' can be reduced.
fixed
[searchtree.cpp:989]: (style) The scope of the variable 'u' can be reduced.
fixed
[searchtree.cpp:77]: (performance) Possible inefficient checking for 'm_Children' emptiness.
no need to fix
[searchtree.cpp:403]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:411]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:460]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:471]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:484]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[searchtree.cpp:510]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[token.cpp:238]: (performance) Possible inefficient checking for 'files' emptiness.
[token.cpp:295]: (performance) Prefer prefix ++/-- operators for non-primitive types.
all no need to fix.
[tokentree.cpp:150]: (style) The scope of the variable 'result' can be reduced.
fixed
[tokentree.cpp:178]: (style) The scope of the variable 'result' can be reduced.
fixed
[tokentree.cpp:210]: (style) The scope of the variable 'result' can be reduced.
fixed
[tokentree.cpp:243]: (style) The scope of the variable 'result' can be reduced.
fixed
[cctreectrl.cpp:86]: (style) C-style pointer casting
[cctreectrl.cpp:166]: (style) C-style pointer casting
[cctreectrl.cpp:167]: (style) C-style pointer casting
[classbrowser.cpp:286]: (style) C-style pointer casting
[classbrowser.cpp:477]: (style) C-style pointer casting
[classbrowser.cpp:492]: (style) C-style pointer casting
[classbrowser.cpp:539]: (style) C-style pointer casting
[classbrowser.cpp:544]: (style) C-style pointer casting
[classbrowser.cpp:807]: (style) C-style pointer casting
Not currently yet. (No need to fix?)
[classbrowserbuilderthread.cpp:66]: (warning) Member variable 'ClassBrowserBuilderThread::m_idThreadEvent' is not initialized in the constructor.
fixed
[codecompletion.cpp:1964]: (performance) Possible inefficient checking for 'result' emptiness.
[codecompletion.cpp:1749]: (performance) Prefer prefix ++/-- operators for non-primitive types.
no need to fix
[coderefactoring.cpp:438]: (style) The scope of the variable 'pos' can be reduced.
fixed
[doxygen_parser.cpp:845]: (performance) Possible inefficient checking for 'tokensIdx' emptiness.
fixed by using empty() function.
[doxygen_parser.cpp:964]: (performance) Possible inefficient checking for 'result' emptiness.
no need to fix
[nativeparser_base.cpp:1217] -> [nativeparser_base.cpp:1219]: (warning) Possible null pointer dereference: token - otherwise it is redundant to check it against null.
This is alrady checked in if (token && MatchType(token->m_TokenKind, kindMask))
[nativeparser_base.cpp:1247] -> [nativeparser_base.cpp:1249]: (warning) Possible null pointer dereference: token - otherwise it is redundant to check it against null.
This is alrady checked in if (token && MatchType(token->m_TokenKind, kindMask))
[nativeparser_base.cpp:869]: (style) Unused variable: autualTypeResult
fixed
[nativeparser_base.cpp:141]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[nativeparser_base.cpp:167]: (performance) Prefer prefix ++/-- operators for non-primitive types.
no need to fix
--- End code ---
The "no need to fix" means I don't think it is necessary to fix them. such as "it++" and "++it" should be the same thing.
The others are that I don't know a correct fix. :)
blauzahn:
Thanks for applying this.
--- Quote ---The "no need to fix" means I don't think it is necessary to fix them. such as "it++" and "++it" should be the same thing.
--- End quote ---
Even then I would change them. Prefer ++it. Make it a habit unless there is a reason to use it++. And it makes usage
of tools like cppcheck easier because it keeps the list short so that the chances to miss a real problem in a long list
are lower. I do not eliminate every warning of a tool slavishly but here it makes sense.
In cases where it++ would actually create a (often unwanted temporary) it may make the program slower.
Rationale: see Herb Sutter C++Coding Standards §28 "[...] prefer calling the prefix forms", §9 "Don't pessimize prematurely."
The same applies to C-style pointer casting. Rationale see Herb Sutter §95 "Don't use C-Style casts."
--- Quote ---
--- Code: ---[cclogger.h:49]: (warning) Member variable 'CCLogger::m_Parent' is not assigned a value in 'CCLogger::operator='.
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_LogId' is not assigned a value in 'CCLogger::operator='.
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_DebugLogId' is not assigned a value in 'CCLogger::operator='.
[cclogger.h:49]: (warning) Member variable 'CCLogger::m_AddTokenId' is not assigned a value in 'CCLogger::operator='.
--- End code ---
Not sure what is a correct fix.
--- End quote ---
As for c++11 I would say for the cctor and op= =default to express the intent of really copying the non-owning pointer member m_Parent
and the ints. Even if the ugly static global variable is the only instance (is it?). In c++98 I would write the cctor explicitly and a canonical
op= and swap. A bit of boilerplate but makes life safer.
Why is there a virtual dtor. Apart from the dtor there is no other virtual method. So, the class is no polymorphic interface and probably
does not need a virtual dtor (including vtable etc.).
Having to remember to call the Init method (from cc ctor) after receiving a pointer to the global variable via Get() is a bit unfortunate since this is
rather bug prone. Since the data-member are private and no method changes any of them I'd make the methods const. If it never changes, may
be Get should return a (cheap and safe) copy instead of a pointer.
oBFusCATed:
--- Quote from: blauzahn on March 10, 2015, 07:43:08 am ---Even then I would change them. Prefer ++it. ...
--- End quote ---
We're using optimizing compilers, so it won't make any difference! If you hate them so much post a patch. It is trivial to do.
And please do it in another topic. This one is related to something else and your post is quite off topic. Also this topic you've started is prone to flaming/religion and so on, so the original one will be swamped by off topic posts!
Fixing warnings is good but fixing real problems that have user visible effects is even better.
@admins: Please separate post related to cppcheck in a separate topic.
ollydbg:
--- Quote from: Huki on March 08, 2015, 08:48:18 am ---...
In ReplaceBufferText() we have:
--- Code: --- if (m_RepeatReplaceCount >= s_MaxRepeatReplaceCount)
{
m_TokenIndex = m_BufferLen - m_FirstRemainingLength;
m_PeekAvailable = false;
SkipToEOL(false);
return false;
}
--- End code ---
Simply change the return to "return true;" and it will fix the problem. It works by skipping (i..e, not parsing) the problem variable "int min;" in the Test struct. If we return false, we stop the current repeat replacement, but later in ParserThread() we will start a new replacement on the same token. :( That's why we get the infinite loop.
...
--- End quote ---
Does return true cause other issue? I don't know. Maybe, we could add some test like
change this
--- Code: ---void Tokenizer::GetReplacedToken(wxString& str)
{
// this indicates we are already in macro replacement mode
if (m_RepeatReplaceCount > 0)
{
--- End code ---
to
--- Code: ---void Tokenizer::GetReplacedToken(wxString& str)
{
// this indicates we are already in macro replacement mode
if (m_RepeatReplaceCount > 0 && m_RepeatReplaceCount < s_MaxRepeatReplaceCount )
{
--- End code ---
But I haven't tested yet.
Navigation
[0] Message Index
[#] Next page
[*] Previous page
Go to full version