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

Fixes for clang #9

Closed
wants to merge 10 commits into from
Closed

Fixes for clang #9

wants to merge 10 commits into from

Conversation

goddessfreya
Copy link
Contributor

These made it compile with clang... you might want to add them.

Signed-off-by: Hal Gentz <[email protected]>
Signed-off-by: Hal Gentz <[email protected]>
Signed-off-by: Hal Gentz <[email protected]>
Signed-off-by: Hal Gentz <[email protected]>
Signed-off-by: Hal Gentz <[email protected]>
Signed-off-by: Hal Gentz <[email protected]>
Signed-off-by: Hal Gentz <[email protected]>
Signed-off-by: Hal Gentz <[email protected]>
@Coolthulhu
Copy link
Member

I don't see any clang problems with current version.
Which version complains and fails to compile?

@goddessfreya
Copy link
Contributor Author

goddessfreya commented Aug 9, 2017

On my pc I get "astyle regressions found.", after running "make astyle" I get what I get from the first commit.
Compiling with "make RELEASE=1 TILES=1 SOUND=1 LOCALIZE=0 CLANG=1 CCACHE=1 -j4" then gives:
"CCACHE_CPP2=1 ccache clang++ -DRELEASE -DGIT_VERSION -DTILES -ffast-math -Os -Wall -Wextra -fsigned-char -Werror -std=c++11 -MMD -MP -D_REENTRANT -I/usr/include/SDL2 -DSDL_SOUND -I/usr/include/SDL2 -D_REENTRANT -D_REENTRANT -I/usr/include/SDL2 -D_REENTRANT -I/usr/include/SDL2 -c src/ranged.cpp -o obj/tiles/ranged.o
src/item_action.cpp:246:30: error: lambda capture 'menu_items' is not used [-Werror,-Wunused-lambda-capture]
const auto sort_menu = [&menu_items]( Iter from, Iter to ) {
^
1 error generated."
And others.

Clang version is "6.0.0 (trunk 310227)", so the warning might not yet be in your version of clang.

@Coolthulhu
Copy link
Member

Looks like you have updated astyle.
At the moment this repo uses the same astyle as CleverRaven one, which is around 2.03-2.05 (the one in Travis - and thus automatic testing is from ubuntu Trusty).
make astyle updates code to current astyle, meaning it will break the code astyled for the version used in repo. So the astyle changes will have to be removed here.

You can override astyle version invoked in make astyle by doing ASTYLE_BINARY="/path/to/binary" make astyle.

@goddessfreya
Copy link
Contributor Author

goddessfreya commented Aug 9, 2017

Travis' logs show this:
"$ make -j3 CCACHE=1 RELEASE=1 CROSS="$CROSS_COMPILATION" && $WINE ./tests/cata_test
/bin/sh: 1: astyle: not found
Cannot run an astyle check, your system either does not have astyle, or it is too old."
Astyle in fact does NOT run on travis. This error happens on your fork and on CleverRaven's.

Another interesting thing is that putting "ASTYLE_BINARY="/home/gentz/Documents/astyle" does NOT stop it from using the system astyle.

And lastly, downgrading my astyle to 2.03 gives me this:
"Cannot run an astyle check, your system either does not have astyle, or it is too old."
and then continues to build fine. Yay?

Anyways. I've reverted the astyle commit. The other commits however are required for clang 6.

@Coolthulhu
Copy link
Member

Another interesting thing is that putting "ASTYLE_BINARY="/home/gentz/Documents/astyle" does NOT stop it from using the system astyle.

I'm not that good with make. I think it may work differently depending on which side of make is that line on. The following two lines could have different results:

ASTYLE_BINARY="/bin/astyle1" make
make ASTYLE_BINARY="/bin/astyle1"

I know that it does overwrite environmental variables, so setting ASTYLE_BINARY in a line without make will most likely not work.

Astyle in fact does NOT run on travis.

Looks like I mistook the Jenkins on CleverRaven for Travis. I'll need to add that then.

The other commits however are required for clang 6.

I'll try to set up the clang 6 to test it then. Travis reports a problem, but it's not on your side (just RNG on Travis side).
Do you mind if I later forward this PR to CleverRaven? It would be less work for me to keep my repo synced that way.
Or you could PR it there and I could merge it from there if you want the "credit".

@goddessfreya
Copy link
Contributor Author

"Looks like I mistook the Jenkins on CleverRaven for Travis. I'll need to add that then."
My bad, you appear to have misunderstood me, or maybe I misunderstood you. Travis tries to do the test but it fails with the error I quoted above error.

"Do you mind if I later forward this PR to CleverRaven? It would be less work for me to keep my repo synced that way."
I don't mind, feel free.

@Coolthulhu
Copy link
Member

All right, I tested it. Had to get experimental clang 6 for that. Forwarded to CleverRaven - if it's rejected there, I'll merge it here, otherwise I'll merge it when syncing.

As for the astyle:
I don't really see how would Travis use astyle newer than 2.05 - this is the astyle in Ubuntu LTS repo.
It could be some user-centric Travis thing. For me it shows no problems with current styling.

@Coolthulhu
Copy link
Member

I forwarded it to cleverraven, where it was merged. Closing here.

@Coolthulhu Coolthulhu closed this Sep 18, 2017
@Trokinos Trokinos mentioned this pull request Oct 30, 2020
Coolthulhu pushed a commit that referenced this pull request Oct 31, 2020
The function `are_requirements_nearby` accesses `player::backlog::front()`, which causes UB when the `backlog` list is empty.

This is usually checked by the caller, but there is at least one way to reach this function without that check:

```
#2  0x000000000083dfaa in string_id<activity_type>::operator== (rhs=..., this=0xdf1b460) at src/player_activity.h:93
#3  are_requirements_nearby (loot_spots=std::vector of length 21, capacity 32 = {...}, needed_things=..., p=..., activity_to_restore=..., in_loot_zones=true, src_loc=...)
    at src/activity_item_handling.cpp:1113
#4  0x0000000000843a46 in generic_multi_activity_check_requirement (p=..., act_id=..., act_info=..., src=..., src_loc=...,
    src_set=std::unordered_set with 1 element = {...}, check_only=false) at src/activity_item_handling.cpp:2676
#5  0x0000000000852813 in generic_multi_activity_handler (act=..., p=..., check_only=check_only@entry=false) at src/activity_item_handling.cpp:2885
#6  0x0000000000800896 in activity_handlers::multiple_butcher_do_turn (act=<optimized out>, p=<optimized out>) at src/activity_handlers.cpp:3822
#7  0x000000000082b248 in std::_Function_handler<void (player_activity*, player*), void (*)(player_activity*, player*)>::_M_invoke(std::_Any_data const&, player_activity*&&, player*&&) (__functor=..., __args#0=<optimized out>, __args#1=<optimized out>) at /usr/include/c++/8/bits/std_function.h:88
#8  0x00000000008630e3 in std::function<void (player_activity*, player*)>::operator()(player_activity*, player*) const (this=<optimized out>, __args#0=<optimized out>,
    __args#0@entry=0xdf1b330, __args#1=<optimized out>, __args#1@entry=0xdf1ae50) at /usr/include/c++/8/bits/std_function.h:260
#9  0x0000000000860306 in activity_type::call_do_turn (this=0x2c3c930, act=act@entry=0xdf1b330, p=p@entry=0xdf1ae50) at src/activity_type.cpp:118
#10 0x00000000014a968b in player_activity::do_turn (this=this@entry=0xdf1b330, p=...) at src/player_activity.cpp:237
#11 0x00000000013092a4 in npc::do_player_activity (this=this@entry=0xdf1ae50) at src/npcmove.cpp:3299
#12 0x0000000001322a07 in npc::execute_action (this=this@entry=0xdf1ae50, action=<optimized out>, action@entry=npc_player_activity) at src/npcmove.cpp:1237
#13 0x000000000132690a in npc::move (this=this@entry=0xdf1ae50) at src/npcmove.cpp:907
```

This adds a simple check within the function.
@Trokinos Trokinos mentioned this pull request Nov 3, 2020
@flesh420 flesh420 mentioned this pull request Oct 4, 2024
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