8

I'm in the final stages of writing a small Calendar library for a school assignment and I have run into an unexpected and very confusing problem; my assignment operator is not overridden when I introduce templates!

So the structure is the following: I have an abstract class Date with mostly pure virtual functions (including the assignment operator), and then I have two subclasses Gregorian and Julian that implement all its functions. Finally, I have written a template for a class Calendar, which contains today's date (in the form of a Gregorian or Julian object) and some other stuff that aren't really relevant for this particular problem.

The problem is that when trying to set this member today I get a long linker error:

Error 4 error LNK2019: unresolved external symbol "public: virtual class lab2::Date & __thiscall lab2::Date::operator=(class lab2::Date const &)" (??4Date@lab2@@UAEAAV01@ABV01@@Z) referenced in function "public: class lab2::Gregorian & __thiscall lab2::Gregorian::operator=(class lab2::Gregorian const &)" (??4Gregorian@lab2@@QAEAAV01@ABV01@@Z) C:\Users...\test.obj Calendar

telling me that it can't find the function operator= in the Date class (obviously because it's purely virtual). Why isn't it using any of the overridden ones? It's telling me that Gregorian::operator= is trying to call Date::operator=?

Here's the simplified code where things go wrong:

namespace cal_lib {
    template <typename T>
    class Calendar {
    public:
        Calendar() {
            today = T(); // this yields the error
        }
    private:
        T today;
    };
 }

And here's a snippet from Gregorian.cpp:

namespace cal_lib {
    class Gregorian : public Date {
    public:
        Gregorian();
        virtual Gregorian& operator=(const Date& date);
        virtual Date& add_year(int n = 1);
        virtual Date& add_month(int n = 1);
    };

    // here I'm using Date's constructor to get the current date
    Gregorian::Gregorian() {}

    Gregorian& Gregorian::operator=(const Date& date) {
        if (this != &date) {
            // these member variables are specified as
            // protected in Date
            m_year = 1858;
            m_month = 11;
            m_day = 17;

            add_year(date.mod_julian_day()/365);
            add_month((date.mod_julian_day() - mod_julian_day())/31);
            operator+=(date.mod_julian_day() - mod_julian_day());
        }
    }
}

The (default) constructor of Date simply sets the values of m_year, m_month and m_day to today's date:

Date::Date() {
    time_t t;
    time(&t);
    struct tm* now = gmtime(&t);
    m_year = now->tm_year + 1900;
    m_month = now->tm_mon + 1;
    m_day = now->tm_mday;
}

It's worth noting though that this works perfectly fine:

Gregorian g;
Julian j;
g = j; // no problems here

Date* gp = new Gregorian();
Date* jp = new Julian();
*gp = *jp; // no problems here either

This is how the Calendar class is instantiated:

using namespace cal_lib;

Calendar<Gregorian> cal;

Is there some very obvious mistake here that I'm doing?

ROMANIA_engineer
  • 54,432
  • 29
  • 203
  • 199
hagward
  • 83
  • 6
  • 4
    What are you sending as the template argument to `cal_lib::Calendar`? – David G Nov 01 '12 at 19:58
  • Do you have explicit `operator=`s for converting between Gregorian and Julian types? – Xymostech Nov 01 '12 at 19:59
  • How `Calendar` is instantiated? – Rost Nov 01 '12 at 20:00
  • As noted by all the questions, we don't have enough information. – Jesse Good Nov 01 '12 at 20:01
  • I was afraid I'd have to give away more code (this is a school assignment after all)... Oh, I forgot the Calendar instantiation, it looks like this: `Calendar cal;` (using namespace cal_lib). – hagward Nov 01 '12 at 20:14
  • Can you post the Gregorian class as well? – David G Nov 01 '12 at 20:17
  • Your examples are not equivalent to your code. You are trying to assign `Gregorian` to `Julian `, but what about assigning smth to `Gregorian`? (`g = j;`, `*gp = *jp`) – Lol4t0 Nov 01 '12 at 20:25
  • @Lol4t0 Ah, you're right about that, I edited that now. I get the same problem with both `Julian` and `Gregorian` (and both `g = j` and `j = g` works fine). – hagward Nov 01 '12 at 20:34
  • Can you post the exact error message (rather than your interpretation of what the error message says)? – David Rodríguez - dribeas Nov 01 '12 at 20:38
  • 2
    I have to ask why you don't just use `Calendar() : today() { }` instead of the default-then-assign method you currently do? Also I wonder if your version of the MS compiler handles the covariant return types properly. – Mark B Nov 01 '12 at 20:40
  • 1
    @MarkB: I would be utterly surprised if the compiler did not handle covariant return types correctly. – David Rodríguez - dribeas Nov 01 '12 at 21:09
  • It is pretty simple. Compiler simply ignores `operator=(const Date&)` of `Gregorian` and prefers `Date`'s one: http://ideone.com/1cTckp. It does not any sense for me now. – Lol4t0 Nov 01 '12 at 21:16
  • @MarkB: Yes, I noticed that doing like that works (and I did so at first). The problem first showed up when I was trying to implement a function for changing the date (i.e., changing `today`). – hagward Nov 01 '12 at 21:30
  • @Lol4t0: I have provided a long answer as to why, but the short one is that it prefers the implicitly declared assignment as it is a better match when both sides of the assignment are of the same type. – David Rodríguez - dribeas Nov 01 '12 at 21:43
  • @DavidRodríguez-dribeas, so I've got it already – Lol4t0 Nov 01 '12 at 21:44

4 Answers4

5

If you carefully read the error message you will notice two things, the compiler is trying to find a definition for:

Date& operator=(const Date&)

And the symbol is needed from the definition of:

Gregorian& operator=(const Gregorian&)

*So how did that operator even appear in your program? *

Copy constructors and assignment operators are special, and they will always be declared in the program. Either you provide a declaration or the compiler will do it for you. You have provided Gregorian& Gregorian::operator=(const Date&) but that does not remove Gregorian& Gregorian::operator=(const Gregorian&) from the program.

When you try to assign one Gregorian object to another, the compiler will find the two overloads yours and the implicitly defined, and overload resolution will find that the implicitly declared assignment is a better match. That will trigger the definition of the assignment operator in a manner similar to:

T& operator=( T const & o ) {
   Base::operator=(o);           // for all bases
   member=o.member;              // for all members
}

There are different things that you can do to solve this issue. The simplest is probably defining Date Date::operator=(const Date&) in your program (leave it as a pure virtual function, but also provide a definition). This way when the compiler encounters two objects of the same derived type it can do its magic and everything will work. You can also use this as means of factoring away the implementation of the copy of the base's members, by forcing dispatch in the derived type.

Another option that takes more effort is to actually declare and define the assignment operator for all of the derived types. There is more code to be written here, and the code that handles the members of Date will need to be duplicated, if you modify the base class you will need to update all of the derived assignment operators... I would not go through that path.

Finally, and once the compiler error is fixed, consider whether your implementation of the assignment operator makes sense or not for a general Date (which might actually be a Gregorian date). Consider what happens if you do:

Gregorian d1 = ...; // some date
Gregorian d2 = d1;  // same date
Date &d = d2;
d1 = d2;            // What date does 'd1' represent??

Note that the issue in this example goes away if your provide the definition of Date::operator= and let the compiler generate Gregorian::operator=(Gregorian const&) for you.

David Rodríguez - dribeas
  • 204,818
  • 23
  • 294
  • 489
  • Oh I see, now the incomprehensible error message finally makes some sense! Thank you so much for all the effort you put into helping me. :) – hagward Nov 01 '12 at 22:11
  • @taperlawyer: Error messages are usually quite explicit, it just takes a couple of times to get used to them and understand them. – David Rodríguez - dribeas Nov 02 '12 at 02:30
2

First of all: While you have public Date& Date::operator=(const Date&) in your Date class, compiler generates implicit function Gregorian& Gregorian::operator=(const Gregorian&) for your Gregorian class.

The compiler-generated function behaves like:

Gregorian& Gregorian::operator=(const Gregorian& g)
{
    Date::operator=(g);
    return *this;
}

According to overload resolution rules, this function is better macth in case

Gregorian g;
Gregorian u;
g = u;

then Date& Date::operator=(const Date&)

If you delete this function or make private, it still will be declared and would be better match for compiler, because compiler ignores accessibility issues when choosing overload:

[Note: The function selected by overload resolution is not guaranteed to be appropriate for the context. Other restrictions, such as the accessibility of the function, can make its use in the calling context ill-formed. —end note ]

(13.3 Overload resolution, C++11 standard draft)

You probably should write realization for Date operator= function. You probably should have same underlining data format for all calendar realization, then you wouldn't need virtual operator=. Now you are trying to perform some conversion, but to do it correctly, you have to know not only type of left operator= operand, but type of right operand also.

Lol4t0
  • 12,444
  • 4
  • 29
  • 65
  • +1 for detecting the overall problem although I don't quite share the *two ways* of solving the issue. The correct approach would be to generate `Date::operator=` to handle the members of the base (less code to write, less error prone, simpler to maintain). In particular assignment of a `Gregorian` date to a different `Gregorian` date is broken in both the question and your implementation. – David Rodríguez - dribeas Nov 01 '12 at 21:48
  • @DavidRodríguez-dribeas. You are right of cause. I hope I now wrote everything correctly. It is a little bit late already and might miss smth. – Lol4t0 Nov 01 '12 at 22:11
1

The simplified code illustrating your problem:

class A {
public:
  virtual A& operator = (const A& a) = 0;
};

class B : public A {
public:
  virtual B& operator = (const A&)
  {
      return *this;
  }
};

int main() 
{
  B b;
  b = B();
}

http://liveworkspace.org/code/add55d1a690d34b9b7f70d196d17657f

Compilation finished with errors:
/tmp/ccdbuBWe.o: In function `B::operator=(B const&)':
source.cpp.cpp:(.text._ZN1BaSERKS_[_ZN1BaSERKS_]+0x14): undefined reference to `A::operator=(A const&)'
collect2: error: ld returned 1 exit status

You think you call this

virtual B& B::operator = (const A&)
//                              ^

but actually what is called is this, auto generated default operator:

  virtual B& B::operator = (const B&)
   //                             ^

Its default implementation calls this operator:

  virtual A& operator = (const A& a) = 0;

which is not implemented - so the error.

The simplest solution is to make explicit cast in assignment:

int main() 
{
  B b;
  b = static_cast<const A&>(B());
}

I would suggest - not to define A operator as virtual - it makes no sense here.

If your really want it virtual, then either implement also its base pure virtual version:

class A {
public:
  virtual A& operator = (const A& a) = 0;
};
inline A& operator = (const A& a) { return *this; }

Or - delete default operator = from all derived from class A - which is very inconvenient to do...

PiotrNycz
  • 23,099
  • 7
  • 66
  • 112
  • I see, thanks a lot for the input! But I wonder, why doesn't it make sense to define A's operator as virtual? In my case, I want all Date objects to have the assignment operator, but depending on the type of the subclass (Gregorian or Julian) it needs to be implemented a little bit differently (or so I figured). Isn't that a case where you want it virtual? – hagward Nov 01 '12 at 22:17
  • @taperlawyer It has no sense in your examples - since you do not use the `Date` as assignable object, like in this example `void assign(Date& a, const Date& b) { a = b; }` - only here this virtual has sense. In this example: `Gregorian g; Julian j; g = j;` - it has no sense since you could define `Gregorian::operator = (Date& a)` without any virtualism. – PiotrNycz Nov 01 '12 at 22:22
0

(obviously because it's purely virtual).

Are you sure? How is it defined exactly? I think that you're defining it wrong. It should look like

virtual Date& operator=(const Date& r) = 0;

note the return value and const Date&.

nothrow
  • 15,882
  • 9
  • 57
  • 104
  • It is defined exactly like that (except that _r_ is named _date_ in my case). In the subclasses the return values are `Gregorian&` and `Julian&` respectively; I've tried changing them to `Date&` as well so that the signatures match exactly but without results. – hagward Nov 01 '12 at 20:09
  • @taperlawyer, see http://stackoverflow.com/questions/669818/virtual-assignment-operator-c for some better explanation of virtual assignments – nothrow Nov 01 '12 at 20:28