eclipse-4diac / 4diac-forte

Eclipse Public License 2.0
30 stars 32 forks source link

Commit "Extracted Device handling into C4diacFORTEInstance class" breaks Zephyr build #155

Closed kaartee closed 4 months ago

kaartee commented 5 months ago

The commit mentioned in the title, 8892bcffb17c2b3c866ba4219d0d71c2206a1004, introduced inconsistencies between header and implementation signatures for forteStopInstance, forteJoinInstance.

azoitl commented 5 months ago

@kaartee an improvement is in work: #151 Feedback is highly appreciated.

kaartee commented 5 months ago

@azoitl In the meantime, couldn't you fix the bug introduced by your commit? It's not just that the signature changed, but forte_Init.h and forte_Init.cpp don't match. I am baffled why the c++ compiler doesn't flag this as an error during compilation of the static forte lib, too:

src/arch/zephyr/forte_Init.h:

FORTE_SHARED_PREFIX void FORTE_SHARED_CALL forteStopInstance(int paSignal, TForteInstance* paResultInstance);
FORTE_SHARED_PREFIX void FORTE_SHARED_CALL forteJoinInstance(TForteInstance* paResultInstance);

src/arch/zephyr/forte_Init.cpp:

void forteStopInstance(int, TForteInstance paInstance) {}
void forteJoinInstance(TForteInstance paInstance) {}
cochicde commented 5 months ago

@azoitl In the meantime, couldn't you fix the bug introduced by your commit? It's not just that the signature changed, but forte_Init.h and forte_Init.cpp don't match. I am baffled why the c++ compiler doesn't flag this as an error during compilation of the static forte lib, too:

src/arch/zephyr/forte_Init.h:

FORTE_SHARED_PREFIX void FORTE_SHARED_CALL forteStopInstance(int paSignal, TForteInstance* paResultInstance);
FORTE_SHARED_PREFIX void FORTE_SHARED_CALL forteJoinInstance(TForteInstance* paResultInstance);

src/arch/zephyr/forte_Init.cpp:

void forteStopInstance(int, TForteInstance paInstance) {}
void forteJoinInstance(TForteInstance paInstance) {}

The problem with the different architectures is that sometimes is hard to try it locally because they require different tools and files to properly compile, so this kind of refactoring are done "blindly". Same as in #151. I tried to cleanup everything and have only one "forte_init.h" (now called just forte.h) so we don't run in these kind of problems again, but I cannot compile and run forte to see if it works, or maybe I can, but I don't know how :(

The possible solutions I see are:

The idea after #151 is that the only thing that might break again in the future other architectures are changes in src/arch/forte.h. The rest would be arch independent

kaartee commented 5 months ago

@cochicde No offense, but I am really just incidentally checking that Forte still builds on Zephyr, as for the time being I have been stopped from working with it. As for the small change introduced by @azoitl , I still hope he will be able to get it right just by taking a second look. I never intended to volunteer as maintainer of all future refactorings that touch the Zephyr port :-) Plus, I never got invited to review the PR before it was eventually merged into develop...

azoitl commented 5 months ago

@kaartee I would clarify a few things here:

  1. We never expected you to be the maintainer of all refactoring
  2. This is the development branch things break here
  3. When I worked on that mentioned PR @cochicde already indicated that he will clean-up afterwards as my use-case worked I was happy for now
  4. We all have to manage our time that we can spend on Eclipse 4diac. So I will not "fix" some code that will anyhow be deleted in the next week. Especially when with the upcoming change an issue like this one will happen much less likely.
kaartee commented 5 months ago

Fixed by #181