Merge lp://staging/~stolowski/unity-scopes-api/canned-query-data into lp://staging/unity-scopes-api/devel

FAILED: Continuous integration, rev:313
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1382/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-ci/92/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/92/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-ci/92/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1382/rebuild

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 + SearchListenerBase::SPtr const& reply) = 0;

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(CannedQueryImpl const &other);
183 CannedQueryImpl(CannedQueryImpl&&) = default;
184 CannedQueryImpl& operator=(CannedQueryImpl const& other) = default;
185 CannedQueryImpl& operator=(CannedQueryImpl&&) = default;

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! :-)

FAILED: Continuous integration, rev:316
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1383/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-ci/93/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/93/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-ci/93/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1383/rebuild

> 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 + SearchListenerBase::SPtr const& reply) = 0;
>
> 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(CannedQueryImpl const &other);
> 183 CannedQueryImpl(CannedQueryImpl&&) = default;
> 184 CannedQueryImpl& operator=(CannedQueryImpl const& other) = default;
> 185 CannedQueryImpl& operator=(CannedQueryImpl&&) = default;
>
> 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::operator=(CannedQuery const& other)
{
    if (this != &other)
    {
        p.reset(new internal::CannedQueryImpl(*(other.p)));
...

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::operator= is not technically required in current implementation, I've added it just in case, to avoid any pitfalls in the future. This obsiously is not called anywhere atm, so shows as not covered by coverage report.

>
> 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.

FAILED: Continuous integration, rev:318
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1384/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-ci/94/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/94/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-ci/94/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1384/rebuild

FAILED: Continuous integration, rev:320
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1385/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-ci/95/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/95/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-ci/95/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1385/rebuild

> 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::operator=(CannedQuery const& other)
> {
> if (this != &other)
> {
> p.reset(new internal::CannedQueryImpl(*(other.p)));
> ...
>
> 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_ptr<int> p;
};

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::operator= is not technically
> 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

> 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.

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 :-(

> 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!

> > 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::operator=(CannedQuery const& other)
> > {
> > if (this != &other)
> > {
> > p.reset(new internal::CannedQueryImpl(*(other.p)));
> > ...
> >
> > 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_ptr<int> p;
> };
>
> 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::operator= is not technically
> > 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.

PASSED: Continuous integration, rev:322
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1390/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-ci/100
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/100
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/100/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-ci/100

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1390/rebuild

PASSED: Continuous integration, rev:323
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1391/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-ci/101
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/101
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/101/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-ci/101

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1391/rebuild

Sorry for being pedantic, but it's still called "query_data" in the following files:

./include/unity/scopes/SearchQueryBase.h
./include/unity/scopes/internal/SearchQueryBaseImpl.h
./src/scopes/SearchQueryBase.cpp
./src/scopes/internal/SearchQueryBaseImpl.cpp

Can we make it consistent please?

> Sorry for being pedantic, but it's still called "query_data" in the following
> files:
>
> ./include/unity/scopes/SearchQueryBase.h
> ./include/unity/scopes/internal/SearchQueryBaseImpl.h
> ./src/scopes/SearchQueryBase.cpp
> ./src/scopes/internal/SearchQueryBaseImpl.cpp
>
> 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.

PASSED: Continuous integration, rev:324
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1394/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-ci/104
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/104
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/104/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-ci/104

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1394/rebuild

Thanks for that! Approving quickly before Jenkins has a change of mind...