Skip to content

Conversation

9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Jul 31, 2013

Changes toUTFz CTFEable.

Additionally fixes a found bug:
Issue 10732 - Example code for std.utf.toUTFindex does not work

if (is(S : const char[]) ||
(isRandomAccessRange!S && is(Unqual!(ElementType!S) == char)))
if (is(S : const char[]) ||
(isRandomAccessRange!S && is(Unqual!(ElementType!S) == char)))
Copy link
Member

Choose a reason for hiding this comment

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

Why are you changing all of this formatting? It has nothing to do with the bugs that you're trying to fix, and the module is fairly consistent the way that it is. All of these format changes are clogging the diff without adding any value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we should improve code format style.

I know there are some opposite opinions about template constraint indentation style.
But from my experience, I think this is practically better.

If template constraint is very long, one level indentation will decrease column count that would be able to use for actual code formatting.

For example, let's see std.algorithm.sort:

SortedRange!(Range, less)
sort(alias less = "a < b", SwapStrategy ss = SwapStrategy.unstable,
        Range)(Range r)
    if (((ss == SwapStrategy.unstable && (hasSwappableElements!Range ||
                                          hasAssignableElements!Range)) ||
         (ss != SwapStrategy.unstable && hasAssignableElements!Range)) &&
        isRandomAccessRange!Range &&
        hasSlicing!Range &&
        hasLength!Range)

VS:

SortedRange!(Range, less)
sort(alias less = "a < b", SwapStrategy ss = SwapStrategy.unstable,
        Range)(Range r)
if (((ss == SwapStrategy.unstable && (hasSwappableElements!Range ||
                                      hasAssignableElements!Range)) ||
     (ss != SwapStrategy.unstable && hasAssignableElements!Range)) &&
    isRandomAccessRange!Range &&
    hasSlicing!Range &&
    hasLength!Range)

If the column limit is 80:

  • With first style, you can use 80-8 columns for the constraint expression formatting.
  • With second style, you can use 80-4 columns for the constraint expression formatting.

This 4-column difference is not small, when considering word wrapping.


Btw, I'm separating commits based on the changing purpose.
You can see diffs on each commits. There's no clogging.

Copy link
Member

Choose a reason for hiding this comment

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

The limit isn't 80. The limit is 120. We have a soft limit of 80 and a hard limit of 120. That seriously mitigates the wrapping problem. It should be relatively rare that function parameters have to go on a second line.

In general, I oppose commits that simply reformat code, especially when it's altering the existing formatting style of the module. Your changes are changing the style of the module, which I object to. We have not agreed on a general style for modules but rather that each module should be internally consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I reverted template constraint indent change. But other style changes (detab and inserting one space after if etc) are kept because they would increase style consistency in std.utf module.

Copy link
Member

Choose a reason for hiding this comment

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

Okay. Thanks.

@monarchdodra
Copy link
Collaborator

The diffs that actually do something (1 & 5) look good to me.
Trivial diffs 3 & 4 Also looks good to me.
Diff 2 I didn't completely process. I saw nothing apart from indent and spacing in there.

So lots of noise, but looks good to me.

jmdavis added a commit that referenced this pull request Aug 1, 2013
@jmdavis jmdavis merged commit 87a2ee7 into dlang:master Aug 1, 2013
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