Skip to content

Conversation

@Abscissa
Copy link
Contributor

@Abscissa Abscissa commented Sep 2, 2011

This puts everything from pull request #156 into one commit.

std/string.d Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This is a problem if C is mutable. It'll work on Windows but not on Linux (since string literals are actually immutable on Linux; I'm not sure why they aren't on Windows).

@andralex
Copy link
Member

andralex commented Sep 3, 2011

I may be missing something, but this implementation is more complicated than it should. If I understand the intent correctly, it should be able to do its job in two passes:

  1. Compute the minimum amount of leading spaces, call it N
  2. drop at most N characters off each line.

There should be no need for e.g. an array of indents and other artifacts. What am I missing?

@andralex
Copy link
Member

I'd suspected the implementation is a fair amount more complicated than it should, and the way I read it it has issues with multibyte characters. So I wrote this:

String[] outdent(String)(String[] lines) if (isSomeString!String)
{
    size_t minLeadingSpaces = size_t.max;
    foreach (line; lines)
    {
        size_t leadingSpaces = 0;
        foreach (dchar c; line)
        {
            if (std.uni.isSpace(c))
            {
                ++leadingSpaces;
            }
            else
            {
                if (leadingSpaces < minLeadingSpaces)
                {
                    minLeadingSpaces = leadingSpaces;
                }
                break;
            }
        }
    }

    if (!minLeadingSpaces || minLeadingSpaces == size_t.max)
    {
        return lines;
    }

    foreach (ref line; lines)
    {
        line.popFrontN(minLeadingSpaces);
    }

    return lines;
}

String outdent(String)(String s) if (isSomeString!String)
{
    auto lines = outdent(splitLines(s));
    s = join(lines, "\n");
    if (s.endsWith('\n')) s ~= '\n';
    return s;
}

You may want to adapt it for this submission. Thanks.

@Abscissa
Copy link
Contributor Author

I've cleaned up the (S str) version as per your suggestions. I've also eliminated the array of indents from the the (S[] lines) version. I don't think it can be significantly simplified further without introducing incorrect behavior upon inconsistent indentations and/or whitespace-only lines.

I also cleaned up the unittests.

I've looked carefully through, and I don't see anything that looks like a potential multibyte-char problem. Is there anything specific you see?

@Abscissa
Copy link
Contributor Author

If you intend to accept #278, then that should probably be done before this is accepted so I can make use of it in outdent without needing to do it in a followup pull request.

@Abscissa
Copy link
Contributor Author

Ok, I'm done with the changes I was planning. Any further issues/comments? If not, should I make it all a single commit again?

@jmdavis
Copy link
Member

jmdavis commented Oct 2, 2011

Looks good to me, so please do rebase so that it's one commit.

@Abscissa
Copy link
Contributor Author

Abscissa commented Oct 2, 2011

Superceded by #282

@Abscissa Abscissa closed this Oct 2, 2011
kuettler pushed a commit to kuettler/phobos that referenced this pull request Feb 6, 2018
Updated makefile to use dmd/generated/os/release/model/dmd
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.

3 participants