Lux-AI-Challenge / Lux-Design-S1

Home to the design and engine of the @Lux-AI-Challenge Season 1, hosted on @kaggle
https://lux-ai.org/
Apache License 2.0
897 stars 152 forks source link

Multiple redefinition of internal lux/ files when importing a header / cpp file combo #56

Closed Desperationis closed 3 years ago

Desperationis commented 3 years ago

Bug: Multiple redefinition of internal lux/ files when importing a header / cpp file combo that both contain lux/kit.hpp.

Minimum possible example from simple/ in kits/cpp

main.cpp

  #include "lux/kit.hpp"
  #include "foo.h"
  #include <string.h>
  #include <vector>
  #include <set>
  #include <stdio.h>

  using namespace std;
  using namespace lux;
  int main()
...

foo.h

  #ifndef FOO_H
  #define FOO_H 
  #include "lux/kit.hpp"
  void foo();
  #endif

foo.cpp

  #include "lux/kit.hpp"
  #include "foo.h"

  void foo() {
    int i = 1 + 1;
  }

Compile command: emcc -s FORCE_FILESYSTEM=1 --pre-js internals/init_fs.js main.cpp foo.cpp -o main.js

Error: https://pastebin.com/5G0Garja

Note: Manually adding header guards to each file in lux , specifically kit.hpp, alleviated most errors. This change has a pull request already. The remaining errors after this change are here: https://pastebin.com/YMJXAFDC

Note 2: This would've thrown an error anyway if you didn't mess with main.js. In fact, this is inherently a C++ problem because if you run g++ main.cpp foo.cpp you get the same error

Possible solution: It's possible to simply move all static variable definitions into a .cpp file in /lux, then put that .cpp file in the compile command and it works just fine as there is now only one definition of those static variables

StoneT2000 commented 3 years ago

to be merged in #51

lufsc commented 3 years ago

I don't think this is fixed. First of all, every header file should have an include guard, as a matter of good style (because someone could include only city.hpp, etc.). If you don't want to type the full include guard every time, you can put #pragma once at the top of the file. This is non-standard, but is supported by every major compiler.

Next, the remaining errors are caused, because (the way it currently is) all free functions inside the header file will be compiled by every source file, which will cause an error when linking, because every object file will contain a definition for these functions. The fix for this is to mark all free functions as inline, which will cause the linker to choose one of these definitions and discard all the others.

// This
Foo foo() {
    return Foo{};
}
// becomes this
inline Foo foo()  {
    return Foo{};
}

Methods defined like this

class Foo {
    void bar() {}
};

are implicitly inline, so they need no special treatment.

Desperationis commented 3 years ago

@lufsc is indeed right; This isn't fixed. Inlining each problematic function is a way for this to be resolved fully. The pull request I made only solved multiple #include's in header files by adding a include guard, but it didn't solve it completely.

In addition to this, lux_io.hpp contains INPUT_CONSTANTS that aren't fixed with a simple include guard; They should be moved to a source file.

lufsc commented 3 years ago

The INPUT_CONSTANTS can be fixed by defining them inline inside the class and removing the out of line definitions.

class Foo {
    static inline int BAR = 5;
}

This requires c++17 though.

Also, most places where you are using static for free functions/variables/constants, you should actually use inline, afaik static in a header makes every source file have its own unique copy of this function, inline uses the same compiled code for every file. So many of the statics should become inlines.

Desperationis commented 3 years ago

That should do it, thank you @lufsc for mentioning that last feature.

Desperationis commented 3 years ago

^ I managed to fix it without C++17 :)