Hi 2015-03-24 13:44 GMT-03:00 Dirk Hohndel <[email protected]>:
> Sorry to be so slow to respond here. > > First, thanks Marcos, solid patches. And good work figuring out an easy > area to work on to get started. > > On Mon, Mar 23, 2015 at 01:47:01PM -0300, Marcos Cardinot wrote: > > Hi Lubomir, > > > > It's not about a dangerous lack of initialization - it's more about code > > consistence. So, the question should be why not initialize all members as > > we should do? > > Well, if the code doesn't initialize the member at all, then that's a bug > and the patch should point that out. Your first patch (the one to > globe.cpp) makes that claim, but that comment is actually incorrect > because eventFilter() will be called before mouseClicked() and it ensures > that doubleClicked is either true or false... > > > In addition, most of these are not following the new coding style (which > we > > discussed in another thread), so if you want to apply them, I could send > > new patches fixing it (and join all changes in a single patch if you > > like)... > > What I would like is a series of patches that > a) follows the newly clarified CodingStyle > b) identifies for each of the changes where the member was previously > initialized and how the new code is better (and "easier to read / > understand" is a valid reason). > > BUT, there are cases where I think having the member initialized makes the > code harder to understand. Let's take your second patch, the one to > updatemanager.cpp. > isAutomaticCheck is ALWAYS initialized BEFORE requestReceived() is > connected - and that's the only way requestReceived() is ever called. > So to me having to different spots that initialize the isAutomaticCheck > member actually makes the code harder to understand because when I look at > requestReceived() it's no longer immediately where this member was set. > > Makes sense? > Yeah, I completely understand your point. In addition, some of them trust that the caller will always set the parameter right after the construction. These cases (do not initialize parameters trusting that some slot, event or setter method will be always called/triggered before anything) are usually a bad programming practice because codes can be added/changed in the future and things like that could produce magic segfaults (more error prone) which would never exists if all members were always initialized in the construction (as recommended by the language)... but it's just my 2c > Marcos, could you create a new patch series that keeps this in mind and > only changes the ones where clarity is improved, and documents the > rationale in the commit message? > Absolutely. Please, see attached patches =D > > /D > >
From 72906c7ef6a125abb024752c63272d507b801ab1 Mon Sep 17 00:00:00 2001 From: Marcos CARDINOT <[email protected]> Date: Wed, 25 Mar 2015 02:41:38 -0300 Subject: [PATCH 1/3] DivelogsDeWebServices::multipart - initializes member Non-static class member multipart is not initialized in the constructor Signed-off-by: Marcos CARDINOT <[email protected]> --- qt-ui/subsurfacewebservices.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/qt-ui/subsurfacewebservices.cpp b/qt-ui/subsurfacewebservices.cpp index fcb565f..99ec509 100644 --- a/qt-ui/subsurfacewebservices.cpp +++ b/qt-ui/subsurfacewebservices.cpp @@ -680,7 +680,9 @@ void DivelogsDeWebServices::uploadDives(QIODevice *dldContent) } } -DivelogsDeWebServices::DivelogsDeWebServices(QWidget *parent, Qt::WindowFlags f) : WebServices(parent, f), uploadMode(false) +DivelogsDeWebServices::DivelogsDeWebServices(QWidget *parent, Qt::WindowFlags f) : WebServices(parent, f), + multipart(NULL), + uploadMode(false) { QSettings s; ui.userID->setText(s.value("divelogde_user").toString()); -- 1.9.1
From 7ba85d0d463c26eb5acea39fb69d8babf0baaa20 Mon Sep 17 00:00:00 2001 From: Marcos CARDINOT <[email protected]> Date: Wed, 25 Mar 2015 02:43:48 -0300 Subject: [PATCH 2/3] DivePlannerPointsModel::recalc - initilizes member Non-static class member recalc is not initialized in the constructor Signed-off-by: Marcos CARDINOT <[email protected]> --- qt-ui/diveplanner.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/qt-ui/diveplanner.cpp b/qt-ui/diveplanner.cpp index db00721..1325b17 100644 --- a/qt-ui/diveplanner.cpp +++ b/qt-ui/diveplanner.cpp @@ -722,6 +722,7 @@ int DivePlannerPointsModel::rowCount(const QModelIndex &parent) const DivePlannerPointsModel::DivePlannerPointsModel(QObject *parent) : QAbstractTableModel(parent), mode(NOTHING), + recalc(false), tempGFHigh(100), tempGFLow(100) { -- 1.9.1
From f40a5521a0bcd99a88bf5f5046f796274c78388f Mon Sep 17 00:00:00 2001 From: Marcos CARDINOT <[email protected]> Date: Wed, 25 Mar 2015 02:48:20 -0300 Subject: [PATCH 3/3] MultiFilterInterface - initilize member + remove extra ; initialize member 'anyChecked' in the constructor Signed-off-by: Marcos CARDINOT <[email protected]> --- qt-ui/filtermodels.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qt-ui/filtermodels.h b/qt-ui/filtermodels.h index 1406b82..9d87241 100644 --- a/qt-ui/filtermodels.h +++ b/qt-ui/filtermodels.h @@ -6,7 +6,7 @@ class MultiFilterInterface { public: - MultiFilterInterface() : checkState(NULL){}; + MultiFilterInterface() : checkState(NULL), anyChecked(false) {} virtual bool doFilter(struct dive *d, QModelIndex &index0, QAbstractItemModel *sourceModel) const = 0; virtual void clearFilter() = 0; bool *checkState; -- 1.9.1
_______________________________________________ subsurface mailing list [email protected] http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
