0011837: Elec weapons (at least the one I'm using) repeat target name

(0032957)
NormalPerson7 (reporter)
2019-01-22 08:19
edited on: 2019-01-22 08:24

I can't replicate this offline and I see no reason why this should be the case online either, looking at the code. Just to be clear, the electricity message is printed in attack.cc line 1524, and the code is:

const char* punctuation = attack_strength_punctuation(special_damage);
special_damage_message = ... ? ... : make_stringf("Lightning courses through %s%s", defender->name(DESC_THE).c_str(), punctuation);

However, if you continue to see this, or anyone else can replicate this, please post it here.

(0032961)
Laret (reporter)
2019-01-22 22:20

Interesting, thanks. I haven't found another elec weapon yet, but it still happens after my save game got upgraded by today's trunk update, and it doesn't happen for a freezing weapon.

(0032962)
Laret (reporter)
2019-01-23 22:55

Started a new game and the same thing is happening with a +2 whip of electrocution.

You whack the goblin!
Lightning courses through the goblingoblin

(0032963)
Laret (reporter)
2019-01-23 22:56

(I can't imagine it matters, but this is on berotato. http://crawl.berotato.org:8080/#play-dcss-git [^] )

(0032964)
Laret (reporter)
2019-01-23 23:15

I suspect I know what's going on here...

Here's the code from 1519-1524 in attack.cc:

            const char *punctuation =
                attack_strength_punctuation(special_damage).c_str();
            special_damage_message =
                defender->is_player()
                ? make_stringf("You are electrocuted%s", punctuation)
                : make_stringf("Lightning courses through %s%s",
                               defender->name(DESC_THE).c_str(), punctuation);

Note that punctuation is the result of calling c_str() on the TEMPORARY string returned by attack_strength_punctuation. If that string has been destroyed before make_string is being called, isn't it reasonable to assume that defender->name(DESC_THE) would be using the same stack block, and, if the short-string optimization is in effect, both c_str() calls might be returning the same address?

If that's the case, the fix would be to save the punctuation string:

            string punctuation =
                attack_strength_punctuation(special_damage);
            special_damage_message =
                defender->is_player()
                ? make_stringf("You are electrocuted%s", punctuation)
                : make_stringf("Lightning courses through %s%s",
                               defender->name(DESC_THE).c_str(), punctuation.c_str());

(0032965)
Laret (reporter)
2019-01-23 23:19

That would explain why only lightning damage does this - all the other damage types have the attack_strength_punctuation(dmg).c_str() call in the make_string statement.

(0032966)
ebering (developer)
2019-01-24 00:15

amalloy agrees with your assessment; I'll note that this doesn't reproduce in clang, but is undefined behaviour. Your suggested change looks like the correct solution (modulo a missed .c_str() call).

(0032967)
amalloy (developer)
2019-01-24 00:17

Fixed in https://github.com/crawl/crawl/commit/16358f6e3a79. [^]

(0032968)
Laret (reporter)
2019-01-24 00:28

Thanks! (What missed .c_str() call? I put the call in the make_string)

(0032969)
ebering (developer)
2019-01-24 00:29

There are two make_stringf calls, you missed make_stringf("You are electrocuted%s", punctuation)

(0032970)
Laret (reporter)
2019-01-24 00:32

D'oh, you are absolutely correct. *blush*

Thanks!

Notes
(0032957) NormalPerson7 (reporter) 2019-01-22 08:19 edited on: 2019-01-22 08:24	I can't replicate this offline and I see no reason why this should be the case online either, looking at the code. Just to be clear, the electricity message is printed in attack.cc line 1524, and the code is: const char* punctuation = attack_strength_punctuation(special_damage); special_damage_message = ... ? ... : make_stringf("Lightning courses through %s%s", defender->name(DESC_THE).c_str(), punctuation); However, if you continue to see this, or anyone else can replicate this, please post it here.

(0032961) Laret (reporter) 2019-01-22 22:20	Interesting, thanks. I haven't found another elec weapon yet, but it still happens after my save game got upgraded by today's trunk update, and it doesn't happen for a freezing weapon.

(0032962) Laret (reporter) 2019-01-23 22:55	Started a new game and the same thing is happening with a +2 whip of electrocution. You whack the goblin! Lightning courses through the goblingoblin

(0032963) Laret (reporter) 2019-01-23 22:56	(I can't imagine it matters, but this is on berotato. http://crawl.berotato.org:8080/#play-dcss-git [^] )

(0032964) Laret (reporter) 2019-01-23 23:15	I suspect I know what's going on here... Here's the code from 1519-1524 in attack.cc: const char *punctuation = attack_strength_punctuation(special_damage).c_str(); special_damage_message = defender->is_player() ? make_stringf("You are electrocuted%s", punctuation) : make_stringf("Lightning courses through %s%s", defender->name(DESC_THE).c_str(), punctuation); Note that punctuation is the result of calling c_str() on the TEMPORARY string returned by attack_strength_punctuation. If that string has been destroyed before make_string is being called, isn't it reasonable to assume that defender->name(DESC_THE) would be using the same stack block, and, if the short-string optimization is in effect, both c_str() calls might be returning the same address? If that's the case, the fix would be to save the punctuation string: string punctuation = attack_strength_punctuation(special_damage); special_damage_message = defender->is_player() ? make_stringf("You are electrocuted%s", punctuation) : make_stringf("Lightning courses through %s%s", defender->name(DESC_THE).c_str(), punctuation.c_str());

(0032965) Laret (reporter) 2019-01-23 23:19	That would explain why only lightning damage does this - all the other damage types have the attack_strength_punctuation(dmg).c_str() call in the make_string statement.

(0032966) ebering (developer) 2019-01-24 00:15	amalloy agrees with your assessment; I'll note that this doesn't reproduce in clang, but is undefined behaviour. Your suggested change looks like the correct solution (modulo a missed .c_str() call).

(0032967) amalloy (developer) 2019-01-24 00:17	Fixed in https://github.com/crawl/crawl/commit/16358f6e3a79. [^]

(0032968) Laret (reporter) 2019-01-24 00:28	Thanks! (What missed .c_str() call? I put the call in the make_string)

(0032969) ebering (developer) 2019-01-24 00:29	There are two make_stringf calls, you missed make_stringf("You are electrocuted%s", punctuation)

(0032970) Laret (reporter) 2019-01-24 00:32	D'oh, you are absolutely correct. blush Thanks!

Issue History
Date Modified	Username	Field	Change
2019-01-22 02:02	Laret	New Issue
2019-01-22 08:19	NormalPerson7	Note Added: 0032957
2019-01-22 08:23	NormalPerson7	Note Edited: 0032957
2019-01-22 08:24	NormalPerson7	Note Edited: 0032957
2019-01-22 22:20	Laret	Note Added: 0032961
2019-01-23 22:55	Laret	Note Added: 0032962
2019-01-23 22:56	Laret	Note Added: 0032963
2019-01-23 23:15	Laret	Note Added: 0032964
2019-01-23 23:19	Laret	Note Added: 0032965
2019-01-24 00:15	ebering	Note Added: 0032966
2019-01-24 00:17	amalloy	Note Added: 0032967
2019-01-24 00:17	amalloy	Status	new => resolved
2019-01-24 00:17	amalloy	Fixed in Branch	=> 0.23 development branch
2019-01-24 00:17	amalloy	Resolution	open => done
2019-01-24 00:17	amalloy	Assigned To	=> amalloy
2019-01-24 00:28	Laret	Note Added: 0032968
2019-01-24 00:29	ebering	Note Added: 0032969
2019-01-24 00:32	Laret	Note Added: 0032970

Relationships