> On Sept. 11, 2013, 6:22 a.m., David Gil Oliva wrote: > > staging/kservice/src/services/kplugininfo.cpp, line 61 > > <http://git.reviewboard.kde.org/r/112660/diff/1/?file=188728#file188728line61> > > > > I think that two variables (hidden and _hidden) so similar are > > confusing. > > Sebastian Kügler wrote: > Hm, I think it's pretty clear, especially since _hidden is defined above > it. The _ signifies here that it's just caching a string. I don't see any > part in the code where it's ambigious. Question: How long did it take you to > understand it? Do others mind this part? (If not, I'd rather drop this issue, > since it's really clear in every call either of these vars is involved.)
Hmmm, I think you're taking it too seriously... I wouldn't have two variables so similar to each other in my code, but if you think it's ok, please drop it! Yes, you're right, I didn't take it too long to understand your code, but I don't think reviews are meant to be so profound. If I see something that I don't see clear, I say so. I think I don't deserve your tone, because I did it with my best intention. > On Sept. 11, 2013, 6:22 a.m., David Gil Oliva wrote: > > staging/kservice/src/services/kplugininfo.cpp, line 67 > > <http://git.reviewboard.kde.org/r/112660/diff/1/?file=188728#file188728line67> > > > > In kdelibs I don't usually see private members in the "_variable" form, > > but "m_variable". > > > > Nevertheless, members of a private class don't usually have any > > underscore or "m_", because when accessing them as d->name makes it clear > > that they are private. Probably you have a reason, though :-) . > > Sebastian Kügler wrote: > Members in the d->pointer usually don't have the m_ prefix, they're just > written in lower case. Maybe you're confusing this with members of the actual > class (which, indeed, use m_ normally)? Members in the d->pointer usually don't have the m_ prefix Yes, I know, that's because I was confused with the _ prefix. Maybe you're confusing this with members of the actual class (which, indeed, use m_ normally)? No, I'm not :-) - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112660/#review39793 ----------------------------------------------------------- On Sept. 10, 2013, 11:32 p.m., Sebastian Kügler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/112660/ > ----------------------------------------------------------- > > (Updated Sept. 10, 2013, 11:32 p.m.) > > > Review request for KDE Frameworks and David Faure. > > > Description > ------- > > This patch prepares KPluginInfo for usage with KPluginTrader (to be submitted > in a separate patch). It basically makes KPluginInfo's API a little bit more > like KService by adding a property(QString) accessor to the plugin info. This > allows us on the one hand > > Part of this patch, and much of its churn, is the internal change from > independent QStrings and QStringLists to a QVariantMap. This is how the > metadata comes in from KPlugin*, and it allows us to make properties > accessible by name. > > There's also a fair bit of moving from QLatin1String to QStringLiteral in > there, most of these lines needed changes anyway. Additionally, the keys of > properties are now shared in the d-pointer. > > This change is source compatible to the old version. > > > Diffs > ----- > > staging/kservice/src/services/kplugininfo.cpp 21e0882 > > Diff: http://git.reviewboard.kde.org/r/112660/diff/ > > > Testing > ------- > > All tests still pass, no regressions encountered otherwise. > > > Thanks, > > Sebastian Kügler > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
