Merge lp://staging/~stolowski/unity-scopes-api/canned-query-data into lp://staging/unity-scopes-api/devel
- canned-query-data
- Merge into devel
Status: | Merged |
---|---|
Approved by: | Michi Henning |
Approved revision: | 324 |
Merged at revision: | 563 |
Proposed branch: | lp://staging/~stolowski/unity-scopes-api/canned-query-data |
Merge into: | lp://staging/unity-scopes-api/devel |
Prerequisite: | lp://staging/~michihenning/unity-scopes-api/qt-coverage |
Diff against target: |
817 lines (+339/-16) 23 files modified
RELEASE_NOTES.md (+1/-0) STRUCTS (+8/-0) debian/changelog (+2/-1) debian/libunity-scopes-qt.symbols (+2/-0) debian/libunity-scopes3.symbols (+6/-1) include/unity/scopes/CannedQuery.h (+27/-1) include/unity/scopes/Scope.h (+20/-0) include/unity/scopes/SearchQueryBase.h (+7/-0) include/unity/scopes/internal/CannedQueryImpl.h (+6/-2) include/unity/scopes/internal/ScopeImpl.h (+8/-0) include/unity/scopes/internal/SearchQueryBaseImpl.h (+1/-0) include/unity/scopes/qt/QCannedQuery.h (+15/-0) include/unity/scopes/qt/internal/QCannedQueryImpl.h (+2/-0) include/unity/scopes/testing/MockScope.h (+7/-0) src/scopes/CannedQuery.cpp (+15/-0) src/scopes/SearchQueryBase.cpp (+15/-4) src/scopes/internal/CannedQueryImpl.cpp (+74/-1) src/scopes/internal/ScopeImpl.cpp (+18/-0) src/scopes/internal/SearchQueryBaseImpl.cpp (+4/-2) src/scopes/qt/QCannedQuery.cpp (+10/-0) src/scopes/qt/internal/QCannedQueryImpl.cpp (+10/-0) test/gtest/scopes/CannedQuery/CannedQuery_test.cpp (+74/-4) test/gtest/scopes/internal/RegistryObject/RegistryObject_test.cpp (+7/-0) |
To merge this branch: | bzr merge lp://staging/~stolowski/unity-scopes-api/canned-query-data |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
PS Jenkins bot (community) | continuous-integration | Approve | |
Michi Henning (community) | Approve | ||
Review via email: mp+250962@code.staging.launchpad.net |
This proposal supersedes a proposal from 2015-02-24.
Commit message
Added support for attaching arbitrary variant data to canned queries.
Description of the change
Added support for attaching arbitrary variant data to canned queries.
Note: needs changes in shell-plugin; a branch for that will follow soon.
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:313
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
Michi Henning (michihenning) wrote : | # |
If you compile with coverage and check CannedQuery and CannedQueryImpl, you'll see a few uncovered parts of the code. It would be nice to add tests for those.
---
145 + virtual QueryCtrlProxy search(std::string const& query_string,
146 + std::string const& department_id,
147 + FilterState const& filter_state,
148 + Variant const& query_data,
149 + SearchMetadata const& metadata,
150 + SearchListenerB
Here (and elsewhere), I'd be inclined to call the parameter "data". That's for consistency with how the getters and setters are named, so we don't have two names for the same thing in different places. We could possibly change it to "user_data" everywhere. I'm thinking that this might be a little more self-describing ("Ah, that's where I can hang my own data"). But I don't feel strongly about it.
---
The tutorial could probably do with an update. No need for an elaborate example, but it would be nice mention somewhere that it's possible to attach arbitrary hunks of data.
---
182 CannedQueryImpl
183 CannedQueryImpl
184 CannedQueryImpl& operator=
185 CannedQueryImpl& operator=
Hmmm... The copy constructor requires a custom implementation, but the copy assignment operator does not? I'm having a very hard time swallowing that ;-)
Copy assignment needs to be implemented too. Astonishingly, this actually compiles, even though the unique_ptr member is not copy assignable. In fact, I tried it with a non-template member that is not copy assignable, just to rule out that it compiled because of non-instantiation of a template member function. But, just as astonishingly, it compiles with a non-assignable non-template member as well. It's perfectly OK to write
C& operator=(C const&) = default;
even if C has data members that are not copy-assignable!
It is *only* if assignment is actually used that the compiler will spit out an error complaining that the class isn't copy assignable. (I tried with both clang and gcc.)
I learn something new and disgusting about this language every week...
All this shows two things:
- If the copy constructor needs overriding, so does the copy assignment operator, and vice versa. Ditto for the move versions. Either override both, or override neither.
- The tests must exercise *all* of the code. That includes copy and move constructors and assignment operators. This is true EVEN if these methods are defined with = default!
---
I'm getting a bit worried about the growing number of overloads. We may need to consider changing to a ParamBuilder approach. I don't want to do this now though. It's a fairly major change to the API.
---
Aside from those few things, nice job, thank you! :-)
- 314. By Paweł Stołowski
-
Implemented copy assignment operator. Renamed *data* to user_data.
- 315. By Paweł Stołowski
-
data -> user_data also in deserialize.
- 316. By Paweł Stołowski
-
More tests.
- 317. By Paweł Stołowski
-
Two more test cases.
- 318. By Paweł Stołowski
-
Updated doc of CannedQuery.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:316
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
- 319. By Paweł Stołowski
-
Test for move.
Paweł Stołowski (stolowski) wrote : | # |
> If you compile with coverage and check CannedQuery and CannedQueryImpl, you'll
> see a few uncovered parts of the code. It would be nice to add tests for
> those.
Ok, added. It's still not 100%, but the new functionality is covered.
>
> ---
>
> 145 + virtual QueryCtrlProxy search(std::string const& query_string,
> 146 + std::string const& department_id,
> 147 + FilterState const& filter_state,
> 148 + Variant const& query_data,
> 149 + SearchMetadata const& metadata,
> 150 + SearchListenerB
>
> Here (and elsewhere), I'd be inclined to call the parameter "data". That's for
> consistency with how the getters and setters are named, so we don't have two
> names for the same thing in different places. We could possibly change it to
> "user_data" everywhere. I'm thinking that this might be a little more self-
> describing ("Ah, that's where I can hang my own data"). But I don't feel
> strongly about it.
Ok, I agree, renamed to user_data.
>
> ---
>
> The tutorial could probably do with an update. No need for an elaborate
> example, but it would be nice mention somewhere that it's possible to attach
> arbitrary hunks of data.
>
Added to CannedQuery class documentation.
> ---
>
> 182 CannedQueryImpl
> 183 CannedQueryImpl
> 184 CannedQueryImpl& operator=
> 185 CannedQueryImpl& operator=
>
> Hmmm... The copy constructor requires a custom implementation, but the copy
> assignment operator does not? I'm having a very hard time swallowing that ;-)
You're right, but in this particular case this omission was technically correct. This is because the copy assignment operator of CannedQuery is forcing copy ctor of CannedQueryImpl, *not* the copy assignment operator of CannedQueryImpl:
CannedQuery& CannedQuery:
{
if (this != &other)
{
p.reset(new internal:
...
This of course doesn't explain why gcc accepted the default copy assignment definition for CannedQueryImpl which now has a non-copy-assignable member.
>
> Copy assignment needs to be implemented too. Astonishingly, this actually
> [...]
Ok, despite the fact that CannedQueryImpl
>
> All this shows two things:
>
> - If the copy constructor needs overriding, so does the copy assignment
> operator, and vice versa. Ditto for the move versions. Either override both,
> or override neither.
>
> - The tests must exercise *all* of the code. That includes copy and move
> constructors and assignment operators. This is true EVEN if these methods are
> defined with = default!
The copy ctror/assignment were already excercised by CannedQuery tests - see TEST(CannedQuery, copy) I've just enhanced these tests to also explicitly check the values of user_data. Also added a new basic test for move ctor.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:318
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
- 320. By Paweł Stołowski
-
Updated symbols.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:320
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
Michi Henning (michihenning) wrote : | # |
> Ok, I agree, renamed to user_data.
> Added to CannedQuery class documentation.
Thanks muchly, I think that'll be easier on the customer's brains :-)
>
> > ---
> >
> You're right, but in this particular case this omission was technically
> correct. This is because the copy assignment operator of CannedQuery is
> forcing copy ctor of CannedQueryImpl, *not* the copy assignment operator of
> CannedQueryImpl:
>
> CannedQuery& CannedQuery:
> {
> if (this != &other)
> {
> p.reset(new internal:
> ...
>
> This of course doesn't explain why gcc accepted the default copy assignment
> definition for CannedQueryImpl which now has a non-copy-assignable member.
Yes. The following shows that the diagnostic is emitted only if assignment is actually used:
class C
{
public:
C();
C(C const&);
C(C&&) = default;
C& operator=(C const&) = default;
C& operator=(C&&) = default;
private:
std:
};
C::C()
: p(new int(42))
{
}
C::C(C const& other)
: p(new int(*other.p))
{
}
int main()
{
C c1;
C c2;
// c1 = c2;
}
This compiles as long the assignment is commented out.
> Ok, despite the fact that CannedQueryImpl
> required in current implementation, I've added it just in case, to avoid any
> pitfalls in the future.
Right! This sort of thing can really cause grief if, some time later, the code is changed to us an assignment somewhere.
> This obsiously is not called anywhere atm, so shows as
> not covered by coverage report.
Why not just add a test that instantiates a CannedQueryImpl and copies/assigns it? You'll get coverage that way.
> The copy ctror/assignment were already excercised by CannedQuery tests - see
> TEST(CannedQuery, copy) I've just enhanced these tests to also explicitly
> check the values of user_data. Also added a new basic test for move ctor.
That test only tests copy, not assignment. There is no assignment there anywhere :-)
CannedQuery b = a;
This isn't an assignment, it's copy construction, even though it looks like assignment.
Same with this:
CannedQuery b = std::move(a); // Calls move constructor
If you want assignment, you need to do:
CannedQuery a(...);
CannedQuery b(...);
b = a; // Copy assignment
b = CannedQuery(...); // Move assignment
b = move(a); // Move assignment
Michi Henning (michihenning) wrote : | # |
> If you want assignment, you need to do:
And, even after jumping through all the hoops, lcov simply leaves any copy constructors and assignment operators that are implemented with "= default" out of the analysis :-(
They are not counted, whether they were actuall called or not. We'll just have to remember to exercise them.
Michi Henning (michihenning) wrote : | # |
BTW, I've pushed your branch with the fixed symbols file here:
lp:~michihenning/+junk/canned-query-data-with-fixed-symbols
Thanks for spotting the problems, and my apologies for having screwed up in the first place. I was careless :-(
- 321. By Paweł Stołowski
-
Merged qt-coverage.
- 322. By Paweł Stołowski
-
Merged symbols fix.
- 323. By Paweł Stołowski
-
Fixed assignment copy/move tests.
Paweł Stołowski (stolowski) wrote : | # |
> BTW, I've pushed your branch with the fixed symbols file here:
>
> lp:~michihenning/+junk/canned-query-data-with-fixed-symbols
>
> Thanks for spotting the problems, and my apologies for having screwed up in
> the first place. I was careless :-(
Thanks a million!
Paweł Stołowski (stolowski) wrote : | # |
> > Ok, I agree, renamed to user_data.
>
> > Added to CannedQuery class documentation.
>
> Thanks muchly, I think that'll be easier on the customer's brains :-)
>
> >
> > > ---
> > >
>
> > You're right, but in this particular case this omission was technically
> > correct. This is because the copy assignment operator of CannedQuery is
> > forcing copy ctor of CannedQueryImpl, *not* the copy assignment operator of
> > CannedQueryImpl:
> >
> > CannedQuery& CannedQuery:
> > {
> > if (this != &other)
> > {
> > p.reset(new internal:
> > ...
> >
> > This of course doesn't explain why gcc accepted the default copy assignment
> > definition for CannedQueryImpl which now has a non-copy-assignable member.
>
> Yes. The following shows that the diagnostic is emitted only if assignment is
> actually used:
>
> class C
> {
> public:
> C();
> C(C const&);
> C(C&&) = default;
> C& operator=(C const&) = default;
> C& operator=(C&&) = default;
>
> private:
> std::unique_
> };
>
> C::C()
> : p(new int(42))
> {
> }
>
> C::C(C const& other)
> : p(new int(*other.p))
> {
> }
>
> int main()
> {
> C c1;
> C c2;
> // c1 = c2;
> }
>
>
> This compiles as long the assignment is commented out.
>
> > Ok, despite the fact that CannedQueryImpl
> > required in current implementation, I've added it just in case, to avoid any
> > pitfalls in the future.
>
> Right! This sort of thing can really cause grief if, some time later, the code
> is changed to us an assignment somewhere.
>
> > This obsiously is not called anywhere atm, so shows as
> > not covered by coverage report.
>
> Why not just add a test that instantiates a CannedQueryImpl and copies/assigns
> it? You'll get coverage that way.
>
> > The copy ctror/assignment were already excercised by CannedQuery tests - see
> > TEST(CannedQuery, copy) I've just enhanced these tests to also explicitly
> > check the values of user_data. Also added a new basic test for move ctor.
>
> That test only tests copy, not assignment. There is no assignment there
> anywhere :-)
>
> CannedQuery b = a;
>
> This isn't an assignment, it's copy construction, even though it looks like
> assignment.
> Same with this:
>
> CannedQuery b = std::move(a); // Calls move constructor
>
> If you want assignment, you need to do:
> [...]
Ah, thanks for that! I've updated these tests.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:322
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:323
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Michi Henning (michihenning) wrote : | # |
Sorry for being pedantic, but it's still called "query_data" in the following files:
./include/
./include/
./src/scopes/
./src/scopes/
Can we make it consistent please?
- 324. By Paweł Stołowski
-
Renamed query_data -> user_data
Paweł Stołowski (stolowski) wrote : | # |
> Sorry for being pedantic, but it's still called "query_data" in the following
> files:
>
> ./include/
> ./include/
> ./src/scopes/
> ./src/scopes/
>
> Can we make it consistent please?
Sure, absolutely makes sense to be consistent, thanks for finding them. According to grep there should be no more of them.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:324
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Michi Henning (michihenning) wrote : | # |
Thanks for that! Approving quickly before Jenkins has a change of mind...
PS Jenkins bot (ps-jenkins) : | # |
FAILED: Continuous integration, rev:308 jenkins. qa.ubuntu. com/job/ unity-scopes- api-devel- ci/1376/ jenkins. qa.ubuntu. com/job/ unity-scopes- api-devel- vivid-amd64- ci/86/console jenkins. qa.ubuntu. com/job/ unity-scopes- api-devel- vivid-armhf- ci/86/console jenkins. qa.ubuntu. com/job/ unity-scopes- api-devel- vivid-i386- ci/86/console
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild: s-jenkins. ubuntu- ci:8080/ job/unity- scopes- api-devel- ci/1376/ rebuild
http://