Skip to content

Issue 13568: Add CT-checked format string overloads to std.format#5288

Merged
dlang-bot merged 3 commits intodlang:masterfrom
ntrel:ct-format
Mar 19, 2017
Merged

Issue 13568: Add CT-checked format string overloads to std.format#5288
dlang-bot merged 3 commits intodlang:masterfrom
ntrel:ct-format

Conversation

@ntrel
Copy link
Contributor

@ntrel ntrel commented Mar 17, 2017

  • Add overloads for formattedWrite, formattedRead, format, sformat.

Check compile-time format strings match the given argument types. I'm using CTFE to evaluate format(fmtStr, Args.init) and see if an exception was thrown; the actual runtime code is unchanged. This is not optimal but makes this PR easy to implement and avoids code duplication/refactoring of formattedWrite. Users can start to pass format strings as compile-time arguments.

  • Throw FormatError when formatValue is called with CTFE and a floating point argument.

This allows snprintf to be avoided in CTFE when checking format strings so floating point arguments can be checked.

@quickfur

* Add overloads for formattedWrite, formattedRead, format, sformat.

* Throw FormatError when formatValue is called with floating points.

The latter allows `snprintf` to be avoided in CTFE when checking format
strings so floating point arguments can be checked.
std/format.d Outdated
try
.format(fmt, Args.init);
catch (Exception e)
return (e.msg is ctfpMessage) ? null : e;
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Are you sure the is will work? Since ctfpMessage is declared as an enum, wouldn't it produce a new instance every time it's referenced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, fixed.

* Params: fmt = Format string. For detailed specification, see $(LREF formattedWrite).
* args = Variadic list of arguments to _format into returned string.
*/
typeof(fmt) format(alias fmt, Args...)(Args args)
Copy link
Member

Choose a reason for hiding this comment

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

Might be a good idea to add sig constraints here to constrain what the user passes in fmt.

Copy link
Member

Choose a reason for hiding this comment

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

Ditto for the other places where alias fmt is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now done. Forgot yesterday :-/

@quickfur quickfur changed the title Add CT-checked format string overloads to std.format Issue 13568: Add CT-checked format string overloads to std.format Mar 17, 2017
@quickfur
Copy link
Member

There's actually a preapproved enhancement ticket for this: https://issues.dlang.org/show_bug.cgi?id=13568

@quickfur
Copy link
Member

quickfur commented Mar 17, 2017

Overall, I very much like this. Once the above comments are addressed, this is good to go.

This is a good first step towards implementing issue 13568. The second half of issue 13568 is not addressed by this PR, that is, to remove dependencies on formatting code that is actually not used by the specified format string. But this is a good first step. Implementing the latter will require extensive refactoring of std.format. In the meantime we can at least get compile-time format string checking working.

Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

Please add the appropriate constraints per @quickfur

* A $(LREF FormatException) if the length of `args` is different
* than the number of format specifiers in `fmt`.
*/
char[] sformat(alias fmt, Args...)(char[] buf, Args args)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this specialized for char?

}

/// ditto
char[] sformat(Char, Args...)(char[] buf, in Char[] fmt, Args args)
Copy link
Member

Choose a reason for hiding this comment

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

Apparently that's why...

@andralex
Copy link
Member

nice work btw

ntrel added 2 commits March 18, 2017 10:33
Add constraints.
Fix test for ctfpMessage equality.
@ntrel
Copy link
Contributor Author

ntrel commented Mar 18, 2017

Thanks for reviews. Added quickfur's suggestions plus a changelog entry.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants