Cleanup and improve std.format #298

Merged
merged 21 commits into from Jan 23, 2012

Conversation

Projects
None yet
5 participants
Member

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.

Member

9rnsr commented Nov 12, 2011

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

Member

9rnsr commented Nov 17, 2011

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

Owner

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?

Member

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.

Owner

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.

Member

9rnsr commented Dec 31, 2011

Updated.

Owner

braddr commented Jan 2, 2012

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

Member

9rnsr commented Jan 2, 2012

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

@jmdavis jmdavis and 1 other commented on an outdated diff Jan 22, 2012

@@ -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 = "[";
@jmdavis

jmdavis Jan 22, 2012

Member

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?

@9rnsr

9rnsr Jan 22, 2012

Member

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.

@jmdavis

jmdavis Jan 22, 2012

Member

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.

@9rnsr

9rnsr Jan 22, 2012

Member

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.

@jmdavis jmdavis and 1 other commented on an outdated diff Jan 22, 2012

@@ -858,7 +904,10 @@ struct FormatSpec(Char)
if (r.front != trailing.front) break;
r.popFront();
}
+ if (!__ctfe) // workaround for strange CTFE error
@jmdavis

jmdavis Jan 22, 2012

Member

If the new __ctfe stuff is a workaround for a CTFE error, rather than a permanent solution, then there should be a comment linking to the associated bug report so that we know when we can remove the workaround.

@9rnsr

9rnsr Jan 22, 2012

Member

Wow, it was no longer needed unawares. I'll remove it.

@jmdavis jmdavis and 1 other commented on an outdated diff Jan 22, 2012

@@ -1068,8 +1047,8 @@ private void formatIntegral(Writer, T, Char)(Writer w, const(T) val, ref FormatS
fs.spec == 'b' ? 2 :
fs.spec == 's' || fs.spec == 'd' || fs.spec == 'u' ? 10 :
0;
- if (base == 0)
- throw new FormatException("integral");
+ enforce(base > 0,
+ new FormatException("integral"));
@jmdavis

jmdavis Jan 22, 2012

Member

Use enforceEx in this sort of situation. e.g.

enforceEx!FormatException(base > 0, "integral");

Now, it looks like FormatException's constructor will need to be adjusted to match

    this(string msg, string file = __FILE__, size_t line = __LINE__, Throwable next = null)

for that to work (it's missing next), but that should probably be done anyway.

@9rnsr

9rnsr Jan 22, 2012

Member

I don't like enforceEx because the condition is far from the head of line.

enforce(condition, throw new SomeException(...));
enforceEx!SomeException(conditoin, ...);   // condition is more important than Exception type name.

But, yes, using enforceEx is better.

@jmdavis jmdavis commented on an outdated diff Jan 22, 2012

@@ -1208,9 +1187,8 @@ if (isFloatingPoint!D)
}
return;
}
- if (std.string.indexOf("fgFGaAeEs", fs.spec) < 0) {
- throw new FormatException("floating");
- }
+ enforce(std.algorithm.find("fgFGaAeEs", fs.spec).length,
+ new FormatException("floating"));
@jmdavis

jmdavis Jan 22, 2012

Member

enforceEx here as well (as well as every other enforce that has new FormatException in it - I'm not going to comment them all).

@jmdavis jmdavis commented on the diff Jan 22, 2012

std/format.d
@@ -1281,135 +1288,92 @@ if (isSomeChar!T)
}
/**
- Strings are formatted like printf does.
+ Strings are formatted like $(D printf) does.
*/
void formatValue(Writer, T, Char)(Writer w, T val, ref FormatSpec!Char f)
if (isSomeString!T && !isStaticArray!T && !is(T == enum))
{
Unqual!(StringTypeOf!T) str = val; // for `alias this`, see bug5371
@jmdavis

jmdavis Jan 22, 2012

Member

Bug# 5371 has been fixed (by you no less).

@jmdavis jmdavis and 1 other commented on an outdated diff Jan 22, 2012

}
unittest
{
- // 5371
- class C1
- {
- const(string) var = "C1";
- alias var this;
- }
- class C2
- {
- string var = "C2";
- alias var this;
- }
- auto c1 = new C1();
- auto c2 = new C2();
+ // 5371 for class
@jmdavis

jmdavis Jan 22, 2012

Member

I take it that these are tests for bug# 5371 and that that's the reason for the comment? Maybe it should be obvious, but I'd change it to something like "Test for bug# 5371 for classes" so that that's clearer.

@9rnsr

9rnsr Jan 22, 2012

Member

I think the comments referencing to bugzilla number is not consistent.
("XXXX", "bug XXXX", "bugzilla XXXX", "@@@bugxxxx@@@", and "bug# XXXX")
Unifying them is good in the future.

@jmdavis jmdavis commented on the diff Jan 22, 2012

std/format.d
}
}
+ return;
@jmdavis

jmdavis Jan 22, 2012

Member

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

@9rnsr

9rnsr Jan 22, 2012

Member

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

@jmdavis jmdavis and 1 other commented on an outdated diff Jan 22, 2012

+ val.popFront();
+ if (val.empty)
+ break;
+ put(w, f.sep);
+ }
+ else
+ {
+ val.popFront();
+ if (val.empty)
+ break;
+ put(w, fmt.trailing);
+ }
+ }
+ }
+ else
+ enforce(false, text("Incorrect format specifier for range: %", f.spec));
@jmdavis

jmdavis Jan 22, 2012

Member

Why not just throw an exception? I don't see much point in using enforce with false.

throw new Exception(text("Incorrect format specifier for range: %", f.spec));
@9rnsr

9rnsr Jan 22, 2012

Member

OK. I'll replace the two enforce(false, ...)s to naked throw statements.

@jmdavis jmdavis and 1 other commented on an outdated diff Jan 22, 2012

{
- formatValue(w, val.re, f);
- put(w, '+');
- formatValue(w, val.im, f);
- put(w, 'i');
-}
+ foreach (i, e; EnumMembers!T)
+ {
+ if (val == e) {
@jmdavis

jmdavis Jan 22, 2012

Member

The brace should be on its own line.

@9rnsr

9rnsr Jan 22, 2012

Member

Oh, thanks.

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.

Member

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...

Member

jmdavis commented Jan 22, 2012

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

Member

9rnsr commented Jan 22, 2012

Applied review result.

Owner

andralex commented Jan 22, 2012

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

@jmdavis jmdavis added a commit that referenced this pull request Jan 23, 2012

@jmdavis jmdavis Merge pull request #298 from 9rnsr/improve_format
Cleanup and improve std.format
59d53d1

@jmdavis jmdavis merged commit 59d53d1 into dlang:master Jan 23, 2012

Member

jmdavis commented Jan 23, 2012

Merged.

Member

9rnsr commented Jan 23, 2012

Thanks for your reviewing and merging, @jmdavis!

Owner

CyberShadow commented Apr 3, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment