> 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.
>
> 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.
> 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)
> 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.
> On May 30, 2013, 2:09 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp, line 428
> > <http://git.reviewboard.kde.org/r/108717/diff/11/?file=145274#file145274line428>
> >
> > QScopedPointer please
>
> Matěj Laitl wrote:
> Dropped? Why? Dropping concerns I raise without any comment discourages
> me from (very time-consuming) reviews.
>
> Edward Hades Toroshchin wrote:
> This is directly linked to the auto_ptr issue above.
Okay, still at least mentioning this would satisfy my expecteancy of "good
behaviour".
More to the point, this is actually a great example why we shouldn't use
std::auto_ptr instead of QScopedPointer: when used once, it creeps into another
places. I'd really dislike half of our codebase using std pointers and another
using Qt ones. We are a Qt project and we've always preferred Qt classes to std
ones. I see no reason to change that.
- 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
