[Kourse 3] Fixing krazy2 issues

Mon Jan 19, 2009 11:19 pm

RE: [Kourse 3] Fixing krazy2 issues

Mon Jan 19, 2009 11:19 pm

msoeken wrote:I had a look into the code of the foreach checker. It seems that QPair is missing as a type to recognize. But your code should be fine. If you want to, you can file a bug report on [url=bugs.kde.org]http://bugs.kde.org[/url]. As I see it, there are just six characters to add to the checker.

It seem they changed the file, this patch is no longer needed. But I'll look at that.

For http://lxr.kde.org/source/KDE/kdeplasma ... ts.cpp#117 , if I do:

Code: Select all: Index: OpenDocuments.cpp =================================================================== --- OpenDocuments.cpp (révision 913834) +++ OpenDocuments.cpp (copie de travail) @@ -113,8 +113,7 @@ QRegExp * extractor = NULL; QString className = task->className(); - SupportedTask st; - foreach (st, m_supportedTasks) { + foreach (const SupportedTask &st, m_supportedTasks) { if (st.m_classPattern.exactMatch(task->className())) { extractor = & st.m_documentNameExtractor; break;

I get:

Code: Select all: /home/kdesvn/kdesvn/kdeplasma-addons/applets/lancelot/app/src/models/OpenDocuments.cpp: In member function ‘bool Models::OpenDocuments::setDataForTask(TaskManager::TaskPtr)’: /home/kdesvn/kdesvn/kdeplasma-addons/applets/lancelot/app/src/models/OpenDocuments.cpp:118: erreur: invalid conversion from ‘const QRegExp*’ to ‘QRegExp*

If I do:

Code: Select all: Index: OpenDocuments.cpp =================================================================== --- OpenDocuments.cpp (révision 913834) +++ OpenDocuments.cpp (copie de travail) @@ -113,10 +113,9 @@ QRegExp * extractor = NULL; QString className = task->className(); - SupportedTask st; - foreach (st, m_supportedTasks) { + foreach (const SupportedTask &st, m_supportedTasks) { if (st.m_classPattern.exactMatch(task->className())) { - extractor = & st.m_documentNameExtractor; + extractor = new QRegExp(st.m_documentNameExtractor); break; } } @@ -140,6 +139,7 @@ description = extractor->cap(2); } + delete extractor; QIcon icon = QIcon(task->icon(32, 32)); set(index, title, description, icon, uint(task->window()));

It works, but is there a better way?

explicit

Code: Select all: Index: lancelot/libs/lancelot/widgets/PopupList.h =================================================================== --- lancelot/libs/lancelot/widgets/PopupList.h (révision 913776) +++ lancelot/libs/lancelot/widgets/PopupList.h (copie de travail) @@ -44,7 +44,7 @@ * Creates a new Lancelot::PopupList * @param parent parent item */ - PopupList(QWidget * parent = 0, Qt::WindowFlags f = Qt::Window); + explicit PopupList(QWidget * parent = 0, Qt::WindowFlags f = Qt::Window); /** * Destroys Lancelot::PopupList

includes

Code: Select all: Index: lancelot/app/src/models/Sessions.h =================================================================== --- lancelot/app/src/models/Sessions.h (révision 913822) +++ lancelot/app/src/models/Sessions.h (copie de travail) @@ -17,8 +17,8 @@ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ -#ifndef LANCELOTAPP_MODELS_SESSION_H -#define LANCELOTAPP_MODELS_SESSION_H +#ifndef LANCELOTAPP_MODELS_SESSIONS_H +#define LANCELOTAPP_MODELS_SESSIONS_H #include "BaseModel.h" #include @@ -44,4 +44,4 @@ } // namespace Models -#endif /* LANCELOTAPP_MODELS_SESSION_H */ +#endif /* LANCELOTAPP_MODELS_SESSIONS_H */ Index: lancelot/libs/lancelot/widgets/ActionListView.h =================================================================== --- lancelot/libs/lancelot/widgets/ActionListView.h (révision 913822) +++ lancelot/libs/lancelot/widgets/ActionListView.h (copie de travail) @@ -23,8 +23,8 @@ #include #include -#include -#include +#include +#include #include #include

qclasses

Code: Select all: Index: bball/bballConfig.ui =================================================================== --- bball/bballConfig.ui (révision 913748) +++ bball/bballConfig.ui (copie de travail) @@ -14,7 +14,7 @@ - + 0

strings

Code: Select all: Index: lancelot/app/src/parts/LancelotPart.cpp =================================================================== --- lancelot/app/src/parts/LancelotPart.cpp (révision 913748) +++ lancelot/app/src/parts/LancelotPart.cpp (copie de travail) @@ -342,7 +342,7 @@ } else if (modelID == "FavoriteApplications") { // We don't want to delete this one (singleton) m_model->addModel(modelID, QIcon(), i18n("Favorite Applications"), model = Models::FavoriteApplications::instance()); - } else if (modelID.startsWith("Folder ")) { + } else if (modelID.startsWith(QString("Folder "))) { modelID.remove(0, 7); m_model->addModel(modelID, QIcon(), Index: weatherstation/lcd.cpp =================================================================== --- weatherstation/lcd.cpp (révision 913748) +++ weatherstation/lcd.cpp (copie de travail) @@ -192,7 +192,7 @@ QString id = element.attribute("id"); if ((pos = id.lastIndexOf(':')) > -1) { groups[id.left(pos)] -1) { Index: twitter/twitter.cpp =================================================================== --- twitter/twitter.cpp (révision 913748) +++ twitter/twitter.cpp (copie de travail) @@ -352,7 +352,7 @@ { //kDebug() kill(); //FIXME only clear it if it was showing an error msg } else { //this is a fake update from a new source @@ -403,7 +403,7 @@ showTweets(); } } - } else if (source.startsWith("Error")) { + } else if (source.startsWith(QString("Error"))) { QString desc = data["description"].toString(); if (desc == "Authentication required"){

syscalls

Code: Select all: Index: lancelot/app/src/models/Places.cpp =================================================================== --- lancelot/app/src/models/Places.cpp (révision 913748) +++ lancelot/app/src/models/Places.cpp (copie de travail) @@ -40,9 +40,9 @@ // We don't want to use addUrl, because of the icons add( i18n("Home Folder"), - getenv("HOME"), + qgetenv("HOME"), KIcon("user-home"), - getenv("HOME") + qgetenv("HOME") ); add(

bruno, proud to be a member of KDE forums since 2008-Oct.

Tue Jan 20, 2009 8:33 am

RE: [Kourse 3] Fixing krazy2 issues

Tue Jan 20, 2009 8:33 am

bruno wrote:
msoeken wrote:I had a look into the code of the foreach checker. It seems that QPair is missing as a type to recognize. But your code should be fine. If you want to, you can file a bug report on [url=bugs.kde.org]http://bugs.kde.org[/url]. As I see it, there are just six characters to add to the checker.

It seem they changed the file, this patch is no longer needed. But I'll look at that.

For http://lxr.kde.org/source/KDE/kdeplasma ... ts.cpp#117 , if I do:
Code: Select all
Index: OpenDocuments.cpp =================================================================== --- OpenDocuments.cpp (révision 913834) +++ OpenDocuments.cpp (copie de travail) @@ -113,8 +113,7 @@ QRegExp * extractor = NULL; QString className = task->className(); - SupportedTask st; - foreach (st, m_supportedTasks) { + foreach (const SupportedTask &st, m_supportedTasks) { if (st.m_classPattern.exactMatch(task->className())) { extractor = & st.m_documentNameExtractor; break;

I get:
Code: Select all
/home/kdesvn/kdesvn/kdeplasma-addons/applets/lancelot/app/src/models/OpenDocuments.cpp: In member function ‘bool Models::OpenDocuments::setDataForTask(TaskManager::TaskPtr)’: /home/kdesvn/kdesvn/kdeplasma-addons/applets/lancelot/app/src/models/OpenDocuments.cpp:118: erreur: invalid conversion from ‘const QRegExp*’ to ‘QRegExp*

If I do:
Code: Select all
Index: OpenDocuments.cpp =================================================================== --- OpenDocuments.cpp (révision 913834) +++ OpenDocuments.cpp (copie de travail) @@ -113,10 +113,9 @@ QRegExp * extractor = NULL; QString className = task->className(); - SupportedTask st; - foreach (st, m_supportedTasks) { + foreach (const SupportedTask &st, m_supportedTasks) { if (st.m_classPattern.exactMatch(task->className())) { - extractor = & st.m_documentNameExtractor; + extractor = new QRegExp(st.m_documentNameExtractor); break; } } @@ -140,6 +139,7 @@ description = extractor->cap(2); } + delete extractor; QIcon icon = QIcon(task->icon(32, 32)); set(index, title, description, icon, uint(task->window()));

It works, but is there a better way?

With the second solution, you still copy the SupportedTask variable which brings no advantage over the initial solution. You just don't have to make it const (but referenced) in the foreach loop:

Code: Select all: foreach (SupportedTask &st, m_supportedTasks) { ... }

[size=x-small]code | [url=cia.vc/stats/author/msoeken]cia.vc[/url] | [url=kde.org/support]donating KDE[/url] | [url=tinyurl.com/cto4ns]wishlist[/url][/size]

Tue Jan 20, 2009 8:49 am

RE: [Kourse 3] Fixing krazy2 issues

Tue Jan 20, 2009 8:49 am

msoeken wrote:With the second solution, you still copy the SupportedTask variable which brings no advantage over the initial solution. You just don't have to make it const (but referenced) in the foreach loop:
Code: Select all
foreach (SupportedTask &st, m_supportedTasks) { ... }

Are you sure we don't need const? I still get an issue from krazy. Also, do you have news for the "Check for cpp macros and usage [cpp]" issues (CMake HAVE_foo macros)?

Code: Select all: Index: lancelot/app/src/models/OpenDocuments.cpp =================================================================== --- lancelot/app/src/models/OpenDocuments.cpp (révision 913851) +++ lancelot/app/src/models/OpenDocuments.cpp (copie de travail) @@ -113,8 +113,7 @@ QRegExp * extractor = NULL; QString className = task->className(); - SupportedTask st; - foreach (st, m_supportedTasks) { + foreach (SupportedTask &st, m_supportedTasks) { if (st.m_classPattern.exactMatch(task->className())) { extractor = & st.m_documentNameExtractor; break;

Code: Select all: ==>For File Type c++<== 1. Check for foreach loop issues [foreach]... 1 issue found lancelot/app/src/models/OpenDocuments.cpp: line#116 (1)

Why should I care? When not using POD types (int, double, pointer, ...) you should use const & for your foreach variables. There are two reasons for this: 1) Prevents you from the mistake of writing foreach loops that modify the list, that is 'foreach(Foo f, list) f.a = f.b = f.c = 0;' compiles but does not modify the contents of list 2) Saves a copy constructor call for each of the list elements

bruno, proud to be a member of KDE forums since 2008-Oct.

Wed Jan 21, 2009 6:58 am

RE: [Kourse 3] Fixing krazy2 issues

Wed Jan 21, 2009 6:58 am

bruno wrote:
msoeken wrote:With the second solution, you still copy the SupportedTask variable which brings no advantage over the initial solution. You just don't have to make it const (but referenced) in the foreach loop:
Code: Select all
foreach (SupportedTask &st, m_supportedTasks) { ... }

Are you sure we don't need const? I still get an issue from krazy. Also, do you have news for the "Check for cpp macros and usage [cpp]" issues (CMake HAVE_foo macros)?

What happens if you write const QRegExp * extractor. Does that work. If it does not, we cannot have const, because when assigning the reference to a pointer it can be changed via the pointer.

[size=x-small]code | [url=cia.vc/stats/author/msoeken]cia.vc[/url] | [url=kde.org/support]donating KDE[/url] | [url=tinyurl.com/cto4ns]wishlist[/url][/size]

Wed Jan 21, 2009 7:13 am

RE: [Kourse 3] Fixing krazy2 issues

Wed Jan 21, 2009 7:13 am

Code: Select all: Index: lancelot/app/src/models/OpenDocuments.cpp =================================================================== --- lancelot/app/src/models/OpenDocuments.cpp (révision 913851) +++ lancelot/app/src/models/OpenDocuments.cpp (copie de travail) @@ -110,11 +110,10 @@ Q_ASSERT(task); // kDebug() className() classClass(); - QRegExp * extractor = NULL; + const QRegExp * extractor = NULL; QString className = task->className(); - SupportedTask st; - foreach (st, m_supportedTasks) { + foreach (const SupportedTask &st, m_supportedTasks) { if (st.m_classPattern.exactMatch(task->className())) { extractor = & st.m_documentNameExtractor; break;

[ 72%] Building CXX object applets/lancelot/app/src/CMakeFiles/plasma_applet_lancelot_part.dir/models/OpenDocuments.o
/home/kdesvn/kdesvn/kdeplasma-addons/applets/lancelot/app/src/models/OpenDocuments.cpp: In member function ‘bool Models::OpenDocuments::setDataForTask(TaskManager::TaskPtr)’:
/home/kdesvn/kdesvn/kdeplasma-addons/applets/lancelot/app/src/models/OpenDocuments.cpp:138: erreur: passing ‘const QRegExp’ as ‘this’ argument of ‘QString QRegExp::cap(int)’ discards qualifiers
/home/kdesvn/kdesvn/kdeplasma-addons/applets/lancelot/app/src/models/OpenDocuments.cpp:139: erreur: passing ‘const QRegExp’ as ‘this’ argument of ‘QString QRegExp::cap(int)’ discards qualifiers
/home/kdesvn/kdesvn/kdeplasma-addons/applets/lancelot/app/src/models/OpenDocuments.cpp: In member function ‘bool Models::OpenDocuments::setDataForTask(TaskManager::TaskPtr)’:
/home/kdesvn/kdesvn/kdeplasma-addons/applets/lancelot/app/src/models/OpenDocuments.cpp:138: erreur: passing ‘const QRegExp’ as ‘this’ argument of ‘QString QRegExp::cap(int)’ discards qualifiers
/home/kdesvn/kdesvn/kdeplasma-addons/applets/lancelot/app/src/models/OpenDocuments.cpp:139: erreur: passing ‘const QRegExp’ as ‘this’ argument of ‘QString QRegExp::cap(int)’ discards qualifiers
make[2]: *** [applets/lancelot/app/src/CMakeFiles/lancelot-menu.dir/models/OpenDocuments.o] Erreur 1

bruno, proud to be a member of KDE forums since 2008-Oct.

Wed Jan 21, 2009 7:18 am

RE: [Kourse 3] Fixing krazy2 issues

Wed Jan 21, 2009 7:18 am

bruno wrote:
Code: Select all
Index: lancelot/app/src/models/OpenDocuments.cpp =================================================================== --- lancelot/app/src/models/OpenDocuments.cpp (révision 913851) +++ lancelot/app/src/models/OpenDocuments.cpp (copie de travail) @@ -110,11 +110,10 @@ Q_ASSERT(task); // kDebug() className() classClass(); - QRegExp * extractor = NULL; + const QRegExp * extractor = NULL; QString className = task->className(); - SupportedTask st; - foreach (st, m_supportedTasks) { + foreach (const SupportedTask &st, m_supportedTasks) { if (st.m_classPattern.exactMatch(task->className())) { extractor = & st.m_documentNameExtractor; break;

[ 72%] Building CXX object applets/lancelot/app/src/CMakeFiles/plasma_applet_lancelot_part.dir/models/OpenDocuments.o
/home/kdesvn/kdesvn/kdeplasma-addons/applets/lancelot/app/src/models/OpenDocuments.cpp: In member function ‘bool Models::OpenDocuments::setDataForTask(TaskManager::TaskPtr)’:
/home/kdesvn/kdesvn/kdeplasma-addons/applets/lancelot/app/src/models/OpenDocuments.cpp:138: erreur: passing ‘const QRegExp’ as ‘this’ argument of ‘QString QRegExp::cap(int)’ discards qualifiers
/home/kdesvn/kdesvn/kdeplasma-addons/applets/lancelot/app/src/models/OpenDocuments.cpp:139: erreur: passing ‘const QRegExp’ as ‘this’ argument of ‘QString QRegExp::cap(int)’ discards qualifiers
/home/kdesvn/kdesvn/kdeplasma-addons/applets/lancelot/app/src/models/OpenDocuments.cpp: In member function ‘bool Models::OpenDocuments::setDataForTask(TaskManager::TaskPtr)’:
/home/kdesvn/kdesvn/kdeplasma-addons/applets/lancelot/app/src/models/OpenDocuments.cpp:138: erreur: passing ‘const QRegExp’ as ‘this’ argument of ‘QString QRegExp::cap(int)’ discards qualifiers
/home/kdesvn/kdesvn/kdeplasma-addons/applets/lancelot/app/src/models/OpenDocuments.cpp:139: erreur: passing ‘const QRegExp’ as ‘this’ argument of ‘QString QRegExp::cap(int)’ discards qualifiers
make[2]: *** [applets/lancelot/app/src/CMakeFiles/lancelot-menu.dir/models/OpenDocuments.o] Erreur 1

Ok, it does not work

I am suggesting to use SupportedTask& as a reference but not const. There are good reasons to do it and perhaps you should put a krazy2 ignore comment on that line.

[size=x-small]code | [url=cia.vc/stats/author/msoeken]cia.vc[/url] | [url=kde.org/support]donating KDE[/url] | [url=tinyurl.com/cto4ns]wishlist[/url][/size]

Wed Jan 21, 2009 7:25 am

RE: [Kourse 3] Fixing krazy2 issues

Wed Jan 21, 2009 7:25 am

msoeken wrote:Ok, it does not work I am suggesting to use SupportedTask& as a reference but not const. There are good reasons to do it and perhaps you should put a krazy2 ignore comment on that line.

What if extractor shouldn't be a pointer?

Code: Select all: Index: lancelot/app/src/models/OpenDocuments.cpp =================================================================== --- lancelot/app/src/models/OpenDocuments.cpp (révision 913851) +++ lancelot/app/src/models/OpenDocuments.cpp (copie de travail) @@ -110,17 +110,16 @@ Q_ASSERT(task); // kDebug() className() classClass(); - QRegExp * extractor = NULL; + QRegExp extractor; QString className = task->className(); - SupportedTask st; - foreach (st, m_supportedTasks) { + foreach (const SupportedTask &st, m_supportedTasks) { if (st.m_classPattern.exactMatch(task->className())) { - extractor = & st.m_documentNameExtractor; + extractor = st.m_documentNameExtractor; break; } } - if (extractor == NULL) { + if (extractor.isEmpty()) { return false; } @@ -135,9 +134,9 @@ QString title = task->visibleName(); QString description; - if (extractor->exactMatch(title)) { - title = extractor->cap(1); - description = extractor->cap(2); + if (extractor.exactMatch(title)) { + title = extractor.cap(1); + description = extractor.cap(2); } QIcon icon = QIcon(task->icon(32, 32));

bruno, proud to be a member of KDE forums since 2008-Oct.

Thu Jan 22, 2009 7:02 am

RE: [Kourse 3] Fixing krazy2 issues

Thu Jan 22, 2009 7:02 am

bruno wrote:
msoeken wrote:Ok, it does not work I am suggesting to use SupportedTask& as a reference but not const. There are good reasons to do it and perhaps you should put a krazy2 ignore comment on that line.

What if extractor shouldn't be a pointer?

Code: Select all
Index: lancelot/app/src/models/OpenDocuments.cpp =================================================================== --- lancelot/app/src/models/OpenDocuments.cpp (révision 913851) +++ lancelot/app/src/models/OpenDocuments.cpp (copie de travail) @@ -110,17 +110,16 @@ Q_ASSERT(task); // kDebug() className() classClass(); - QRegExp * extractor = NULL; + QRegExp extractor; QString className = task->className(); - SupportedTask st; - foreach (st, m_supportedTasks) { + foreach (const SupportedTask &st, m_supportedTasks) { if (st.m_classPattern.exactMatch(task->className())) { - extractor = & st.m_documentNameExtractor; + extractor = st.m_documentNameExtractor; break; } } - if (extractor == NULL) { + if (extractor.isEmpty()) { return false; } @@ -135,9 +134,9 @@ QString title = task->visibleName(); QString description; - if (extractor->exactMatch(title)) { - title = extractor->cap(1); - description = extractor->cap(2); + if (extractor.exactMatch(title)) { + title = extractor.cap(1); + description = extractor.cap(2); } QIcon icon = QIcon(task->icon(32, 32));

Now we call the copy constructor on QRegExp, but it looks like the best solution for now.

[size=x-small]code | [url=cia.vc/stats/author/msoeken]cia.vc[/url] | [url=kde.org/support]donating KDE[/url] | [url=tinyurl.com/cto4ns]wishlist[/url][/size]

Thu Jan 22, 2009 7:05 am

RE: [Kourse 3] Fixing krazy2 issues

Thu Jan 22, 2009 7:05 am

I will now close this kourse. Thanks to all students for their great work. Many patches have been send and so many krazy2 issues could have been fixed. Hopefully you could learned something about krazy2, sending patches and registering to mailing lists. I know that some of you got a svn account after sending some krazy2 patches. If you want to give some feedback (good and bad) to this kourse, feel free to reply it to the discussion thread.

Cheers, m.

Last edited by msoeken on Thu Jan 22, 2009 10:19 am, edited 1 time in total.

[size=x-small]code | [url=cia.vc/stats/author/msoeken]cia.vc[/url] | [url=kde.org/support]donating KDE[/url] | [url=tinyurl.com/cto4ns]wishlist[/url][/size]

Who is online