Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix build with Apple Clang #98

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Yadunund
Copy link
Member

@Yadunund Yadunund commented Jan 25, 2023

Depends on open-rmf/rmf_utils#27

When building on macOS with the default Apple Compiler, several build errors are thrown as seen below (small sample)

  virtual std::shared_ptr<const ParticipantDescription> get_participant(
                                                        ^
/Users/yadunund/ws_rmf/src/rmf/rmf_traffic/rmf_traffic/include/rmf_traffic/schedule/Viewer.hpp:134:40: note: unimplemented pure virtual method 'get_itinerary' in 'Database'
  virtual std::optional<ItineraryView> get_itinerary(
                                       ^
/Users/yadunund/ws_rmf/src/rmf/rmf_traffic/rmf_traffic/include/rmf_traffic/schedule/Viewer.hpp:140:33: note: unimplemented pure virtual method 'get_current_plan_id' in 'Database'
  virtual std::optional<PlanId> get_current_plan_id(

It looks like the base classes such as Viewer or ItineraryViewer have virtual methods with ParticipantID as argument type whereas classes that implement these methods such as Database or Mirror replace ParticipantId with std::size_t. As a result the compiler think we're wrongly implementing the virtual methods.

I'm actually surprised gcc or llvm-clang doesn't catch this same error?
I'm also assuming this change will break API/ABI so perhaps this should not be backported into humble

Yadunund added 2 commits January 24, 2023 02:09
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
@Yadunund Yadunund requested a review from mxgrey January 25, 2023 12:31
@codecov
Copy link

codecov bot commented Jan 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 23.01%. Comparing base (8274fd2) to head (177138d).

Current head 177138d differs from pull request most recent head 2cd027f

Please upload reports for the commit 2cd027f to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #98      +/-   ##
==========================================
+ Coverage   21.71%   23.01%   +1.30%     
==========================================
  Files         361      369       +8     
  Lines       30302    30769     +467     
  Branches    14200    14371     +171     
==========================================
+ Hits         6580     7083     +503     
- Misses      14218    14235      +17     
+ Partials     9504     9451      -53     
Flag Coverage Δ
tests 22.89% <100.00%> (+1.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
..._traffic/include/rmf_traffic/schedule/Database.hpp 0.00% <ø> (ø)
...mf_traffic/include/rmf_traffic/schedule/Mirror.hpp 0.00% <ø> (ø)
rmf_traffic/src/rmf_traffic/schedule/Database.cpp 49.23% <ø> (+1.98%) ⬆️
rmf_traffic/src/rmf_traffic/schedule/Mirror.cpp 41.79% <ø> (+1.58%) ⬆️
rmf_traffic/test/unit/schedule/test_Spacetime.cpp 38.71% <100.00%> (+8.80%) ⬆️

... and 149 files with indirect coverage changes

Signed-off-by: Luca Della Vedova <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants