Jump to page: 1 2
Thread overview
[Issue 7942] Appending different string types corrupts memory
Jul 29, 2014
yebblies
Jul 29, 2014
yebblies
Jul 30, 2014
Kenji Hara
Jul 30, 2014
yebblies
Jul 30, 2014
Martin Nowak
Aug 06, 2014
Vladimir Panteleev
Aug 06, 2014
yebblies
Aug 12, 2014
Vladimir Panteleev
Aug 15, 2014
Walter Bright
Sep 09, 2014
yebblies
July 29, 2014
https://issues.dlang.org/show_bug.cgi?id=7942

yebblies <yebblies@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Appending a string to a     |Appending different string
                   |dstring is allowed          |types corrupts memory

--
July 29, 2014
https://issues.dlang.org/show_bug.cgi?id=7942

yebblies <yebblies@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |pull
           Assignee|nobody@puremagic.com        |yebblies@gmail.com

--- Comment #3 from yebblies <yebblies@gmail.com> ---
I'm not so sure about this being accepts-invalid.  D allows implicit string decoding/encoding in the single-char case, and it seems reasonable to support it here.

These patches change it from a wrong-code to a mere performance issue.

https://github.com/D-Programming-Language/dmd/pull/3831 https://github.com/D-Programming-Language/druntime/pull/915

--
July 30, 2014
https://issues.dlang.org/show_bug.cgi?id=7942

--- Comment #4 from github-bugzilla@puremagic.com ---
Commits pushed to master at https://github.com/D-Programming-Language/druntime

https://github.com/D-Programming-Language/druntime/commit/d0f8bca7ad1eea1b8e9e2774fc84a1af7ce73815 Issue 7942 - Appending different string types corrupts memory

https://github.com/D-Programming-Language/druntime/commit/d0e3b25d74e1f73cd12a916d0ccf00da2f4082ab Merge pull request #915 from yebblies/issue7942

Issue 7942 - Appending different string types corrupts memory

--
July 30, 2014
https://issues.dlang.org/show_bug.cgi?id=7942

--- Comment #5 from Kenji Hara <k.hara.pg@gmail.com> ---
(In reply to yebblies from comment #3)
> I'm not so sure about this being accepts-invalid.  D allows implicit string decoding/encoding in the single-char case, and it seems reasonable to support it here.

In contrast to the one character transcoding on foreach iteration, the implicit transcoding cost on appending will be bigger when appended string is very long, and it would be sometimes difficult to find it.

I think accepting it is not reasonable.

--
July 30, 2014
https://issues.dlang.org/show_bug.cgi?id=7942

--- Comment #6 from yebblies <yebblies@gmail.com> ---
(In reply to Kenji Hara from comment #5)
> 
> In contrast to the one character transcoding on foreach iteration, the implicit transcoding cost on appending will be bigger when appended string is very long, and it would be sometimes difficult to find it.
> 
> I think accepting it is not reasonable.

Appending an array of values to an array will take longer than appending a single value.  Even with implicit re-encoding it will still be O(N), same as other arrays.

Appending an array of structs with postblits could easily cause the same sort of performance problems.

--
July 30, 2014
https://issues.dlang.org/show_bug.cgi?id=7942

Martin Nowak <code@dawg.eu> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |code@dawg.eu

--- Comment #7 from Martin Nowak <code@dawg.eu> ---
It's only a performance problem if it was easy to do inadvertently. Where would this be the case?

When done deliberately the cost implications are fairly obvious.

--
August 06, 2014
https://issues.dlang.org/show_bug.cgi?id=7942

hsteoh@quickfur.ath.cx changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hsteoh@quickfur.ath.cx

--- Comment #8 from hsteoh@quickfur.ath.cx ---
I see two solutions to this bug: (1) transcode the string to be appended (or,
more generally, convert each value to be appended into the target type), which
is costly (but still only O(n)); (2) reject this kind of array appending at
compile-time.

I'm leaning towards (1) as being more user-friendly, going along D's motto of "correct by default, efficient if you ask for it". (2) is a bit too restrictive IMO, given the amount of implicit castings that we already have.

One possible compromise is to have (2) suggest std.array.appender for concatenating strings of different element widths.

--
August 06, 2014
https://issues.dlang.org/show_bug.cgi?id=7942

Vladimir Panteleev <thecybershadow@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |thecybershadow@gmail.com

--- Comment #9 from Vladimir Panteleev <thecybershadow@gmail.com> ---
Another option would be to fix but deprecate the functionality.

> going along D's motto of "correct by default, efficient if you ask for it"

I don't think that means that hidden costs are OK.

--
August 06, 2014
https://issues.dlang.org/show_bug.cgi?id=7942

--- Comment #10 from yebblies <yebblies@gmail.com> ---
(In reply to Vladimir Panteleev from comment #9)
> Another option would be to fix but deprecate the functionality.
> 
> > going along D's motto of "correct by default, efficient if you ask for it"
> 
> I don't think that means that hidden costs are OK.

O(n) append is not a hidden cost - it's the standard.  If we're ok with the single-char case, we should be fine with the string case too.

--
August 12, 2014
https://issues.dlang.org/show_bug.cgi?id=7942

--- Comment #11 from hsteoh@quickfur.ath.cx ---
Looks like we're at an impasse. I think the best thing we can do right now is to make appending strings of different widths illegal for now. That still leaves open the option that in the future, we could support auto-conversion. But in any case, the current situation should not continue, since it introduces memory corruption.

--
« First   ‹ Prev
1 2