mohitanand001 / underscore_cpp

underscore_cpp
MIT License
7 stars 30 forks source link

Some header file declarations in underscore.hpp are inconsistent with definitions in underscore.cpp #12

Open mohitanand001 opened 5 years ago

srinathkp commented 5 years ago

Can I work on this?

mohitanand001 commented 5 years ago

Sure @srinath9721

mohitanand001 commented 5 years ago

@srinath9721 also make sure the order of functions is same in cpp and hpp files. Please ping if any help is needed.

mohitanand001 commented 5 years ago

@srinath9721 are you still working on this. Kindly keep us updated.

gubatron commented 5 years ago

I'm working on a somewhat related branch.

I'm introducing formal tests and when I try to use the functions in my test cases, I'm having a lot of issues linking the test executable.

Things like:

Undefined symbols for architecture x86_64:
  "std::__1::vector<int, std::__1::allocator<int> > _::set_union<std::__1::vector<int, std::__1::allocator<int> >, std::__1::vector<int, std::__1::allocator<int> > >(std::__1::vector<int, std::__1::allocator<int> >, std::__1::vector<int, std::__1::allocator<int> >, std::__1::vector<int, std::__1::allocator<int> >)", referenced from:
      ____C_A_T_C_H____T_E_S_T____0() in testing.cpp.o
ld: symbol(s) not found for architecture x86_64

And it all comes from clang warnings like:

/Users/gubatron/workspace.frostwire/underscore_cpp/tests/testing.cpp:11:28: warning: instantiation of function '_::set_union<std::__1::vector<int, std::__1::allocator<int> >, std::__1::vector<int, std::__1::allocator<int> > >' required here, but no definition is available [-Wundefined-func-template]
        std::vector<int> sec = _::set_union(vec, rec, lec);
                                  ^
/Users/gubatron/workspace.frostwire/underscore_cpp/src/underscore.hpp:62:15: note: forward declaration of template entity is here
    Container set_union(Container container1, Container container2, Containers  ... others);
              ^
/Users/gubatron/workspace.frostwire/underscore_cpp/tests/testing.cpp:11:28: note: add an explicit instantiation declaration to suppress this warning if '_::set_union<std::__1::vector<int, std::__1::allocator<int> >, std::__1::vector<int, std::__1::allocator<int> > >' is explicitly instantiated in another translation unit
        std::vector<int> sec = _::set_union(vec, rec, lec);

Then I decided to make some tests with a basic templated function in underscore.hpp and underscore.cpp

// underscore.hpp
template<typename T>
void say_foo(T foo);

and then in the .cpp file

// underscore.cpp
template<typename T>
void say_foo(T foo) {
  std::cout << foo << std::endl;
}

This would cause the same issues when trying to do:

_::say_foo("Hello");
_::say_foo(1337);

The problem exists because the compiler doesn't know of declarations for:

void _::say_foo(std::string foo);
void _::say_foo(int foo);

The easy solution to it all is to get rid of the .cpp code for templated functions and put the code in the .hpp, for the case of this library that'd mean moving all implementations to the .hpp and getting rid of the .cpp file, thus making the project a header only library.

Otherwise it requires doing a bunch of forward declarations for each possible data type case use of each templated function

If we move everything to the .hpp, then this issue is also solved, no inconsistencies would exist if there was a single file.

Edit: See: https://isocpp.org/wiki/faq/templates#templates-defn-vs-decl https://isocpp.org/wiki/faq/templates#separate-template-fn-defn-from-decl

mohitanand001 commented 5 years ago

Even I have faced a similar issue (not sure when). I am not sure, if it would be considered a "good practice" to keep both the implementation and headers in same file. Anyway your call.

gubatron commented 5 years ago

I'll try to research if there's a way to keep them separated and not have the linkage issues. For now I'll leave it as a header only file, which for now would make it very simple for people to use, just bring a single .hpp file.

mohitanand001 commented 5 years ago

Cool