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

Fix warnings #2804

Merged
merged 2 commits into from
Apr 25, 2020
Merged

Fix warnings #2804

merged 2 commits into from
Apr 25, 2020

Conversation

elsid
Copy link
Collaborator

@elsid elsid commented Apr 24, 2020

No description provided.

elsid added 2 commits April 25, 2020 01:38
@elsid
Fix warning -Wrange-loop-construct
975e4f9 
apps/openmw/mwgui/loadingscreen.cpp:81:36: warning: loop variable 'extension' of type 'const std::__cxx11::basic_string<char>' creates a copy from type 'const std::__cxx11::basic_string<char>' [-Wrange-loop-construct]
                    for(auto const extension: supported_extensions)
                                   ^
apps/openmw/mwgui/loadingscreen.cpp:81:25: note: use reference type 'const std::__cxx11::basic_string<char> &' to prevent copying
                    for(auto const extension: supported_extensions)
                        ^~~~~~~~~~~~~~~~~~~~~
                                   &
@elsid
Use std::array for a list of extensions
ffacc30 
This list doesn't change and the size is known at compile time.
@akortunov akortunov merged commit 528fc58 into OpenMW:master Apr 25, 2020
@elsid elsid deleted the fix_warnings branch April 25, 2020 13:12
@AnyOldName3
Copy link
Member

Two points:

  • I've not used std::array much, but it's raising a mental red flag that there's a hardcoded 7 as the array length. Surely the compiler should be able to infer the length parameter from the fact that we're initialising the array with {{ stuff }}.
  • It might just be a better idea to ask osgDB which readers it has that can return osg::Image.

@elsid
Copy link
Collaborator Author

elsid commented May 18, 2020

Class template arguments deduction is a C++17 feature: https://en.cppreference.com/w/cpp/container/array/deduction_guides, so it's not possible to use it now. Also even with C++17 partial argument deduction (std::array<std::string> v{"foo", "bar"}) will not work and probably will never be added to C++ standard. If you're interested here is explanation why.

@psi29a
Copy link
Member

psi29a commented May 18, 2020

We can bump to C++17 and see. So long as MacOS accepts (the rest shouldn't be a problem) it, then we're good to go. Right now, the only thing really holding us back was that std::filesystem wasn't yet supported, but that doesn't mean that other things haven't been implemented.

Perhaps it's worth a PR to bump C++17 and give it at try?

@AnyOldName3
Copy link
Member

Android's still missing chunks of C++17

@psi29a
Copy link
Member

psi29a commented May 19, 2020 •
edited
Loading

Yes, so is MacOS pre 10.15

As I said, we can give it a try and see what we can get away with given the circumstances.

However, while we are waiting on travis to catch up... I would assume that Android NDK has at least been updated by now? Do we have a status report?

According to this:
https://developer.android.com/ndk/guides/cpp-support

Paging @xyzz : have any comments on this?

@xyzz
Copy link
Contributor

xyzz commented May 19, 2020

android/ndk#609 is still open so that's at least one part that's missing. Not sure if there's anything else.

@psi29a
Copy link
Member

psi29a commented May 19, 2020 •
edited
Loading

@xyzz could you test if this is available as of r21 ?

https://en.cppreference.com/w/cpp/container/array/deduction_guides

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@elsid @AnyOldName3 @psi29a @xyzz @akortunov