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.

Sign up for GitHub

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

Jump to bottom

Removed un-needed calls to std::move #5561

Merged
merged 2 commits into from
Oct 1, 2019
Merged

Removed un-needed calls to std::move #5561

merged 2 commits into from
Oct 1, 2019

Conversation

peoplestom
Copy link
Contributor

@peoplestom peoplestom commented Sep 19, 2019 •
edited
Loading

These calls were throwing a pessimistic move error and stopping compilation on GCC 9.1.0

Issue

5560 pessimistic std::move calls

Tasklist

Requirements / Relations

N/A

@peoplestom
Copy link
Contributor Author

First github PR in a while, let me know if I've missed something in the process.

gardster
gardster requested changes Sep 19, 2019
View reviewed changes
Copy link
Contributor

@gardster gardster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution!

Please add GCC9.10 build to the CI rules https://github.com/Project-OSRM/osrm-backend/blob/master/.travis.yml

src/server/api/parameters_parser.cpp Show resolved Hide resolved
include/updater/csv_file_parser.hpp Show resolved Hide resolved
@peoplestom
Copy link
Contributor Author

I believe I've now added gcc9 to Travis with my last push

@gardster
Copy link
Contributor

Thank you! It seems that CI found additional errors. Could you fix them?

@peoplestom
Copy link
Contributor Author

I missed that, I was working off the 5.22.0 tag. I have fixed the additional issues, it compiles and appears to work.
As I'm not familiar with flatbuffer's and that's were the issues were from could someone who know's what they're looking at review this?
It looks like @akashihi may have done the initial flatbuffer work.

@peoplestom
Removed un-needed calls to std::move
018a9bc 
These calls were throwing a pessimistic move error and stopping compilation.
@akashihi
Copy link
Contributor

I'll take a look on that flatbuffers issue next week. Hope it is not urgent :)

@akashihi
Copy link
Contributor

akashihi commented Oct 1, 2019

Added #5566 on top of master that fixes flatbuffers caused compilation warnings

@gardster
Copy link
Contributor

gardster commented Oct 1, 2019

@peoplestom Thank you for the fixes.
Can you format
include/engine/api/base_api.hpp include/engine/api/match_api.hpp include/engine/api/route_api.hpp with clang-format to pass formatting tests?
Then I can merge this PR. You can find formatting script in scripts/format.sh but make sure that you have proper version (or at least old enough version) of the clang-format.

@peoplestom
Copy link
Contributor Author

I have clang-format 8.0.1, but the changes it kicked up were right were my edits were, so fingers crossed this is ready to merge!

@peoplestom
Fixed flatbufferbuiler copy issues.
2889537 
Compiling under gcc9.1 we get copy issues.
It appears we shouldn't pass builder classes by value, only ref.
@gardster
Copy link
Contributor

gardster commented Oct 1, 2019

Thank you for your contribution! Now OSRM will be gcc9 compatible 🎉

@gardster gardster merged commit 0205cbc into Project-OSRM:master Oct 1, 2019
@peoplestom
Copy link
Contributor Author

Awesome! glad I could contribute

datendelphin added a commit to fossgis-routing-server/osrm-backend that referenced this pull request Nov 19, 2020
@datendelphin
Merge tag 'v5.23.0'
a823897 
  - Changes from 5.22.0
    - Build:
      - FIXED: pessimistic calls to std::move [Project-OSRM#5560](Project-OSRM#5561)
    - Features:
      - ADDED: new API parameter - `snapping=any|default` to allow snapping to previously unsnappable edges [Project-OSRM#5361](Project-OSRM#5361)
      - ADDED: keepalive support to the osrm-routed HTTP server [Project-OSRM#5518](Project-OSRM#5518)
      - ADDED: flatbuffers output format support [Project-OSRM#5513](Project-OSRM#5513)
      - ADDED: Global 'skip_waypoints' option [Project-OSRM#5556](Project-OSRM#5556)
      - FIXED: Install the libosrm_guidance library correctly [Project-OSRM#5604](Project-OSRM#5604)
      - FIXED: Http Handler can now deal witch optional whitespace between header-key and -value [Project-OSRM#5606](Project-OSRM#5606)
    - Routing:
      - CHANGED: allow routing past `barrier=arch` [Project-OSRM#5352](Project-OSRM#5352)
      - CHANGED: default car weight was reduced to 2000 kg. [Project-OSRM#5371](Project-OSRM#5371)
      - CHANGED: default car height was reduced to 2 meters. [Project-OSRM#5389](Project-OSRM#5389)
      - FIXED: treat `bicycle=use_sidepath` as no access on the tagged way. [Project-OSRM#5622](Project-OSRM#5622)
      - FIXED: fix table result when source and destination on same one-way segment. [Project-OSRM#5828](Project-OSRM#5828)
      - FIXED: fix occasional segfault when swapping data with osrm-datastore and using `exclude=` [Project-OSRM#5844](Project-OSRM#5844)
      - FIXED: fix crash in MLD alternative search if source or target are invalid [Project-OSRM#5851](Project-OSRM#5851)
    - Misc:
      - CHANGED: Reduce memory usage for raster source handling. [Project-OSRM#5572](Project-OSRM#5572)
      - CHANGED: Add cmake option `ENABLE_DEBUG_LOGGING` to control whether output debug logging. [Project-OSRM#3427](Project-OSRM#3427)
      - CHANGED: updated extent of Hong Kong as left hand drive country. [Project-OSRM#5535](Project-OSRM#5535)
      - FIXED: corrected error message when failing to snap input coordinates [Project-OSRM#5846](Project-OSRM#5846)
    - Infrastructure
      - REMOVED: STXXL support removed as STXXL became abandonware. [Project-OSRM#5760](Project-OSRM#5760)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gardster gardster gardster approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@peoplestom @gardster @akashihi