> On May 30, 2013, 2:09 p.m., Matěj Laitl wrote: > > src/core-impl/collections/nepomukcollection/NepomukCollection.cpp, lines > > 40-41 > > <http://git.reviewboard.kde.org/r/108717/diff/11/?file=145267#file145267line40> > > > > Okay, personal preferences aside, I'm sure that consistency within a > > project has more value than preferences of individual developers. > > > > There are about 25 .cpp files in current Amarok source code that embody > > all the method definitions into namespace XYZ {}. On the other hand, there > > are about 265 files that use using namespace XYZ; syntax for function > > definitions. Please improve, rather than fight, consistency. > > Edward Hades Toroshchin wrote: > It's not about consistency. Favouring the using-directive is not a > question of style or preference. > > Matěj Laitl wrote: > So it is a question of what? What is the (technical, now that you've > excluded personal one) rationale for introducing this incosistency? (yes, I > still view it as an inconsistency) > > I also don't like "issues" being marked are resolved here without ack of > the one who opened them. > > Edward Hades Toroshchin wrote: > About marked issues: reviewboard's workflow allows that, and we don't > have any policy against it. My view is that this code will be maintained > mostly by me, so any opened issues do not have a veto power over the patch, > unless the issue creator specifically mentions, that the code with this issue > unresolved is unacceptable. > > Regarding the using-directive specifically: the semantics of > using-directive do not allow it to be a viable replacement for placing the > definitions inside the namespace. Therefore consistency is not important. > > > Matěj Laitl wrote: > > and we don't have any policy against it. > > Well, I'd consider it just a good behaviour, which hopefully doesn't have > to be adopted as another "policy". But I acknowledge that what is considered > a "good behaviour" is culturally-dependent. > > > My view is that this code will be maintained mostly by me > > This assumption is always false. You are not working on your personal > project. This is a community project, members of the community should listen > to concerns raised by other members, else the community doesn't really work > right. > > > Regarding the using-directive specifically: the semantics of > using-directive do not allow it to be a viable replacement for placing the > definitions inside the namespace. > > This is true, in general, but in all of "namespace Something {" > occurrences in .cpp files in this patch there is no difference. Not a > deal-breaker of course, but I'd appreciate more cooperative behaviour. > > Edward Hades Toroshchin wrote: > > what is considered a "good behaviour" is culturally-dependent > > Did you mean that as an insult? > > > This is a community project, members of the community should listen to > concerns raised by other members, else the community doesn't really work > right. > > Which I do. I also believe members of the community should not put spokes > into each others' wheels. > > > in all of "namespace Something {" occurrences in .cpp files in this > patch there is no difference > > Which is obviously completely irrelevant. > > > I'd appreciate more cooperative behaviour > > How more cooperative could this get?
> Did you mean that as an insult? Not at all, sorry it could have been interpreted as it. (still I'd like to encourage closing rb issues only after discussion) > Which is obviously completely irrelevant. I don't see why, but okay, I see you insist on keeping the code as-is, enough energy was spent discussing this very small detail. > On May 30, 2013, 2:09 p.m., Matěj Laitl wrote: > > src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h, line > > 46 > > <http://git.reviewboard.kde.org/r/108717/diff/11/?file=145268#file145268line46> > > > > We prefer Qt equivalents of std classes, if they exist. But reading > > documentation of std::auto_ptr, it doesn't seem to make sense as a function > > argument, you doesn't seem to want to delete it as soon as it goes out of > > scope (and yeah, it may be reset by being on rhs of an assignment, but that > > is convoluted). > > Edward Hades Toroshchin wrote: > std::auto_ptr makes perfect sense as a function argument, because it both > clearly shows who is responsible for the pointer deallocation and enforces it. > > Matěj Laitl wrote: > Not for me. This isn't usual for other Amarok code and I wouldn't like to > see such practice introduced. The current practice it to document pointer > ownership properly in docstrings. > > I'd like to see this issue reopened. (ditto about closing issues without > reviewer's ack) > > Edward Hades Toroshchin wrote: > Self-documenting and self-policing interfaces are good, even if they > aren't used often. > > (ditto about issues policy) > > Matěj Laitl wrote: > > Self-documenting and self-policing interfaces are good, even if they > aren't used often. > > I agree, but this could at least use QScopedPointer, which AFAICS is 1:1 > equivalent to std::auto_ptr, modulo some method names. It has also more > explicit behaviour (::take() instead of rhs-modifying assignment behaviour), > which I actually consider more self-documenting that current solution. > > I still think memory management should be rather documented explicitly in > this case. > > Edward Hades Toroshchin wrote: > No it isn't equivalent. Quite unfortunately, because I don't like mixing > std and Qt. > > However, if you still fail to understand the approach I've used here, > I'll replace it with something simpler. > However, if you still fail to understand the approach I've used here, I'll > replace it with something simpler. This is very likely, so please do. All the time I thought this just serves to say NepomukInquirer takes ownership of parser. Was it the case? - Matěj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108717/#review33230 ----------------------------------------------------------- On May 30, 2013, 7:12 p.m., Edward Hades Toroshchin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/108717/ > ----------------------------------------------------------- > > (Updated May 30, 2013, 7:12 p.m.) > > > Review request for Amarok and Vishesh Handa. > > > Description > ------- > > nepomuk: implement custom QueryMaker > > > Diffs > ----- > > src/core-impl/collections/nepomukcollection/CMakeLists.txt > 642919bd7b2188c6055308eabc07319ae48e14be > src/core-impl/collections/nepomukcollection/NepomukCache.h PRE-CREATION > src/core-impl/collections/nepomukcollection/NepomukCache.cpp PRE-CREATION > src/core-impl/collections/nepomukcollection/NepomukCollection.h > 5737cb5861563a8f190a29badc15d9e3ef82faf1 > src/core-impl/collections/nepomukcollection/NepomukCollection.cpp > 8c35e3dda0e3dc9a23affe1d18cc69de4d24d58c > src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h > 66e2473f16227f462c5a3838c71f311b6594333a > src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.cpp > 19743fe02b1d0c9dd4785d01909a4231b3a69e90 > src/core-impl/collections/nepomukcollection/NepomukInquirer.cpp > PRE-CREATION > src/core-impl/collections/nepomukcollection/NepomukParser.h PRE-CREATION > src/core-impl/collections/nepomukcollection/NepomukParser.cpp PRE-CREATION > src/core-impl/collections/nepomukcollection/NepomukQueryMaker.h > PRE-CREATION > src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp > PRE-CREATION > src/core-impl/collections/nepomukcollection/NepomukSelectors.h PRE-CREATION > src/core-impl/collections/nepomukcollection/meta/NepomukAlbum.h > 8456ddb8d952e130816d01fc312c8df3146e83d1 > src/core-impl/collections/nepomukcollection/meta/NepomukAlbum.cpp > 76c9136f9d4f5ba77cac49b242b3aa3b2d844c0c > src/core-impl/collections/nepomukcollection/meta/NepomukArtist.h > 7cf533e760b0a06e5a2316693e13f6aea0ac3dcd > src/core-impl/collections/nepomukcollection/meta/NepomukArtist.cpp > 34d7c8f9c32f96be0ef5f81de9d58bfedb510c2d > src/core-impl/collections/nepomukcollection/meta/NepomukComposer.h > 837c37fa5dce5c3c10e6840f618cf72430b51b3d > src/core-impl/collections/nepomukcollection/meta/NepomukComposer.cpp > 33ba85ae11100e8ccbb5cc94a2d7d0e2007fdbd1 > src/core-impl/collections/nepomukcollection/meta/NepomukGenre.cpp > c34831ab4812f0241c0000eef844c2ea2397ae97 > src/core-impl/collections/nepomukcollection/meta/NepomukLabel.h > 6f2e54c3c6a0d350615cea0e335ed9f5c5a0eedf > src/core-impl/collections/nepomukcollection/meta/NepomukLabel.cpp > 7e2a313e5eac91128d0ce211c6575ac3d8d2b1ab > src/core-impl/collections/nepomukcollection/meta/NepomukTrack.h > 9a534dddac51fc39f9d3705f95b194f9669416ab > src/core-impl/collections/nepomukcollection/meta/NepomukTrack.cpp > b9beca872cd7c7edc3e6b15662d0a49f42213c6d > src/core-impl/collections/nepomukcollection/meta/NepomukYear.h > 9eca44fbcd3aab77149d864cb6d3e5d266cc8461 > src/core-impl/collections/nepomukcollection/meta/NepomukYear.cpp > 1d6e3b1066e13fd6adba15ae6b1d81f8ca28a5d9 > src/statsyncing/collection/CollectionProvider.cpp > 7e3bdd83214796fc37e0aef41225baedabe3fbda > > Diff: http://git.reviewboard.kde.org/r/108717/diff/ > > > Testing > ------- > > Playing tracks from Nepomuk collection, browsing, filtering, adding/removing > labels, statistics sync. > > > File Attachments > ---------------- > > > > http://git.reviewboard.kde.org/media/uploaded/files/2013/02/03/nepomuk-counting-querymaker.png > > > http://git.reviewboard.kde.org/media/uploaded/files/2013/02/03/nepomuk-queries.png > > > http://git.reviewboard.kde.org/media/uploaded/files/2013/02/03/nepomuk-sync.png > > > Thanks, > > Edward Hades Toroshchin > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
