Skip to content

Conversation

9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Oct 20, 2011

Points:

  • Cleanup template specializations for more maintainable.

  • Add toString(sink) support.

    It is useful when format specifier does not need.

  • Add format specifier %| for nested range formatting

    Previously, In %( xxx %s yyy %) yyy is interpreted as separator implicitly.
    But this rule is not good for nested range formatting.
    e.g. {%(<%(%d%)>, %)} with [[1,2,3], [4,5,6]] makes {<123>, <456}.

    New %| format specifiers explicitly divide format strings for element and for separator.
    It is used only inside %( and %)
    e.g. {%(<%(%d%)>%|, %)} with [[1,2,3], [4,5,6]] makes {<123>, <456>}.

  • Add '%c" specifier for character type formatting

    It is already supported for unformatting.
    And also, it is necessary for nested range formatting. Now, inside compound format specifier, %s runs proper element formatting (do escape for strings, characters, and ranges). It is useful, but in some cases we need a way to avoid escaping. %c just do it for characters.

  • Issue 5354 - formatValue: range templates introduce 3 bugs related to class & struct cases

    Check toString is overridden in runtime for class input range object.

  • Add toString(sink, ...) support for class and interface types

  • Add tests and improvements for reflective formatting.

    'Reflective' means:

    1. format value.
    2. unformat formatted string with same specifier.
    3. the result value equals to original

    And now, reflective formatting is CTFEable, except floating point formatting.

@9rnsr
Copy link
Contributor Author

9rnsr commented Nov 12, 2011

Make reflective formattings CTFEable. Thanks for Don's fixing dmd!

@9rnsr
Copy link
Contributor Author

9rnsr commented Nov 17, 2011

OK, now we can test this pull without any compiler fixes.

@andralex
Copy link
Member

andralex commented Dec 4, 2011

I'm not very fond of "%|", it adds complication. But I guess there's no way without it or something like it. The problem is there's no way to specify a terminator, for example you can't format the array [1, 2, 3] as "1;2;3;" and the empty array as "". If you force adding a terminator after the format specifier a la "%(%s;%);", then the empty array is represented as ";". Yuk. Any ideas for a simpler approach?

@9rnsr
Copy link
Contributor Author

9rnsr commented Dec 4, 2011

The problem is there's no way to specify a terminator, for example you can't format the array [1, 2, 3] as "1;2;3;" and the empty array as "".

You can use "%(%s;%|%)". It has explicit terminator ;, and empty separator.

@braddr
Copy link
Member

braddr commented Dec 29, 2011

Looks like this has bit rotten a little.. merge conflicts.

What's left to get this pull committed? It's been open a good while.

@9rnsr
Copy link
Contributor Author

9rnsr commented Dec 31, 2011

Updated.

@braddr
Copy link
Member

braddr commented Jan 2, 2012

Now fails due to some delegate related issues, likely due to recent changes..

@9rnsr
Copy link
Contributor Author

9rnsr commented Jan 2, 2012

I've already posted #593 for more escape check.

@@ -572,25 +591,25 @@ struct FormatSpec(Char)
This string is inserted before each sequence (e.g. array)
formatted (by default $(D "[")).
*/
static const(Char)[] seqBefore = "[";
enum const(Char)[] seqBefore = "[";
Copy link
Member

Choose a reason for hiding this comment

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

Why aren't these enums immutable? const buys us nothing here as far as I can tell. e.g.

enum immutable Char[] seqAfter = "]";

Shouldn't all of these variables that you just changed from static to enum be changed to be immutable like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing I had thought is "Change formatting with global state is less useful. They should be only default values for sequence formating". So I had converted them to manifest constant simply.
Therefore, I can agree to change const to immutable. But

enum immutable Char[] seqAfter = "]";

causes "conflicting storage class immutable" error. So I'll change them to

enum immutable(Char)[] seqAfter = "]";

like string type.

Copy link
Member

Choose a reason for hiding this comment

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

If fully immutable doesn't work, that's fine, though with the recent changes to IFTI, it's generally less of a problem than it was. It doesn't really matter in this case though. You end up with a new copy of the enum every time you use it anyway. It's being const instead of immutable that's the real issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's being const instead of immutable that's the real issue.

?? These manifest constants are dynamic array, and the compiler stores the content of string literal at most once into exe file.
So, yes, manifest constant creates a copy in each place that used, but it is 8 byte structure (in 32 bit system), not full copy of string.

I think the change from const(Char)[] to immutable(Char)[]` is better, because it expresses the elements never modified.
But it is not real issue.

}
}
return;
Copy link
Member

Choose a reason for hiding this comment

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

Why bother with the return; when it's already at the end of the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, there is not at the end of the function. The return statement is need.

@jmdavis
Copy link
Member

jmdavis commented Jan 22, 2012

There are a lot of calls to enforce in std.format which don't use FormatException (though not all). Is there a reason for this? Normally, I would expect code in Phobos to throw an exception related to its module (and possibly one more specific to the error that it's reporting), not `Exception." It's too generic.

@9rnsr
Copy link
Contributor Author

9rnsr commented Jan 22, 2012

As I wrote in above comment, I don't like enforceEx because the condition that's the most important part does not stand out. But also, I can agree enforce is too generic than enforceEx and using it is better.

I had thought to define alias enforceEx!SomeException XXX, but it was impossible...

@jmdavis
Copy link
Member

jmdavis commented Jan 22, 2012

alias enforce!SomeException is not enough to instantiate the template. The first argument is templated is well.

@9rnsr
Copy link
Contributor Author

9rnsr commented Jan 22, 2012

Applied review result.

@andralex
Copy link
Member

Jonathan, since you spent time getting into this diff, you may want to merge it when ready.

jmdavis added a commit that referenced this pull request Jan 23, 2012
Cleanup and improve std.format
@jmdavis jmdavis merged commit 59d53d1 into dlang:master Jan 23, 2012
@jmdavis
Copy link
Member

jmdavis commented Jan 23, 2012

Merged.

@9rnsr
Copy link
Contributor Author

9rnsr commented Jan 23, 2012

Thanks for your reviewing and merging, @jmdavis!

@CyberShadow
Copy link
Member

This pull request introduced a regression:
https://d.puremagic.com/issues/show_bug.cgi?id=12505

marler8997 pushed a commit to marler8997/phobos that referenced this pull request Nov 10, 2019
Remove digger setup and use the compiler from Travis
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.

5 participants