[WIP] function argument stringification: __ARG_STRING__ (replaced by 7821) by timotheecour · Pull Request #7811 · dlang/dmd

timotheecour · 2018-01-30T22:10:40Z

Stringify caller's function argument, analog to C's # stringification macro (except it's type safe).
A main use case is to allow finally writing nice logging and debug tools.

Purely library based alternatives all have drawbacks, see #7811 (comment) for example.

It also works with UFCS.

Eg:

string log(T)(T a, string name=__ARG_STRING__!a)
{
  import std.conv;
  return text(name, ": ", a);
}
assert(log(1+2) == "1 + 2: 3");
int x=3;
assert(log(x) == "x: 3");

NOTE:

code calls earg.stringified = earg.toChars; for all function arguments only when attempting to match (via callMatch) against a function that has __ARG_STRING__. Caveat: enum folding will happen before that, so enum x=3; assert(log(x) == "3: 3"); instead of assert(log(x) == "x: 3"); ; but the behavior is correct for non-enum's (eg static const)
currently only allows binding to a function parameter (not template parameter, and not variadic parameters); this could be lifted in future PR without breaking backward compatibility

EDIT: closing in favor of #7821

dlang-bot · 2018-01-30T22:10:41Z

Thanks for your pull request, @timotheecour! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

wilzbach · 2018-01-30T23:42:12Z

Wow, I wanted to add this to DMD to, but you beat me to it.
However, I can add further motivation for this. With __ARG_NAME__ we can finally write nice logging and debug tools.
Consider, for example, my stalled PR to Phobos (dlang/phobos#4318) for adding a user-friendly dump method, which never went anywhere because the best we can do at the moment was:

dump!(x, y) // x == 5, y == 6
dump(x, y) // arg.1= 5, arg.2 = 6

Both have severe downsides, __ARG_NAME__ is ideal for this use case.
Other potential areas of use are of course at least:

logging (e.g. stdx.logger, vibe.core.logger, ..)
better exception messages ("myFancyVar is null" is a lot more helpful than "r is null")

timotheecour · 2018-01-31T00:32:06Z

yes, that's exactly of of the main use cases i had in mind :) will update commit msg to make it clearer

jacob-carlborg · 2018-01-31T07:40:44Z

Please no. This is yet another hack because we don't have AST macros. Yes, I know that we most likely will not get those.

I'll give it a fair review anyway.

jacob-carlborg · 2018-01-31T07:41:48Z

changelog/argname.dd

+
+Eg:
+---
+string log(T)(T a, string name=__ARG_NAME__!a)


The code implements __ARG_STRING__.

jacob-carlborg · 2018-01-31T07:42:38Z

src/dmd/expression.h

    TOK op;                     // to minimize use of dynamic_cast
    unsigned char size;         // # of bytes in Expression so we can copy() it
    unsigned char parens;       // if this is a parenthesized expression
+    const(char)* stringified;  // for function calls (in case of __ARG_NAME__)


Same here, the code implements __ARG_STRING__.

jacob-carlborg · 2018-01-31T07:46:57Z

test/runnable/testargnames.d

+  // Checks that no constant folding happens, cf D20180130T161632.
+  fun1(1+1+2, `1 + 1 + 2`);
+  enum t=44;
+  fun1(t+t+t, `t + t + t`); // want: t+t+t


Operators should have spaces on each side.

jacob-carlborg · 2018-01-31T07:50:42Z

I would like to see examples/tests what happens in the following scenarios:

__ARG_STRING__ is used in a non-parameter context
More complex expressions
Expressions with user defined types
Passing a type to __ARG_STRING__
Using __ARG_STRING__ as the default value of a template argument

jacob-carlborg

There are many issues with the code style. These are the corrections:

Space after if
Spaces around operators
else on its own line
for -> foreach
Four spaces for indentation

jacob-carlborg · 2018-01-31T07:53:46Z

src/dmd/tokens.d

        TOK.overloadSet: "__overloadset",
        TOK.file: "__FILE__",
        TOK.fileFullPath: "__FILE_FULL_PATH__",
+        TOK.argumentName: "__ARG_STRING__",


The naming is inconsistent. The token is called argumentName while the symbol used in the code is called __ARG_STRING__.

jacob-carlborg · 2018-01-31T07:54:31Z

src/dmd/parse.d

    /*****************************************
     * Parses default argument initializer expression that is an assign expression,
-     * with special handling for __FILE__, __FILE_DIR__, __LINE__, __MODULE__, __FUNCTION__, and __PRETTY_FUNCTION__.
+     * with special handling for __FILE__, __FILE_DIR__, __LINE__, __MODULE__, __FUNCTION__, __PRETTY_FUNCTION__, __ARG_NAMES__.


__ARG_NAMES__ -> __ARG_STRING__.

jacob-carlborg · 2018-01-31T08:01:32Z

src/dmd/expression.d


+/***********************************************************
+ */
+extern (C++) final class ArgnameInitExp : DefaultInitExp


Now there are three different names in play here: ArgnameInitExp, __ARG_STRING__ and argumentName. Pick one of them and use it consistently. Please also avoid abbreviations.

changed to more consistent naming:

__ARG_STRING__ argString ArgStringInitExp isArgStringInitExp resolveArgString

I did use commonly used abbreviation though, __ARGUMENT_STRING__ would be too long IMO (and some ppl had suggesed ARG also)

jacob-carlborg · 2018-01-31T08:02:56Z

src/dmd/expressionsem.d

+                  assert(nargs>0);
+                  bool found=false;
+                  // TODO: should Parameters._foreach be used?
+                  for(int u=0;u<nparams;u++)


Use foreach instead of for.

jacob-carlborg · 2018-01-31T08:04:50Z

src/dmd/expressionsem.d

            return;
        }

+        for(int i=0;i<(exp.arguments ? exp.arguments.dim : 0) ; i++){


asDArray can be used instead.

not used anymore

jacob-carlborg · 2018-01-31T08:07:41Z

There are commits from other people that are already merged.

…ify computed from candidate matching function

timotheecour · 2018-01-31T08:37:57Z

There are commits from other people that are already merged.

fixed

timotheecour · 2018-01-31T09:27:47Z

ARG_STRING is used in a non-parameter context

I am disallowing this use case; ARG_STRING is useful mostly as a default function argument. If you have a good use case for that (which one, I'd be curious?) we could always add that later.

Using ARG_STRING as the default value of a template argument

as noted in PR msg, this is on TODO list but could be done in subsequent PR while being backward compatible

More complex expressions
Expressions with user defined types

added this:

fun1(A.init.x + A(a).myfun(), `A(1).x + A(a).myfun()`);

see tests

…mplex expressions

jacob-carlborg · 2018-01-31T09:46:02Z

I am disallowing this use case; ARG_STRING is useful mostly as a default function argument. If you have a good use case for that (which one, I'd be curious?) we could always add that later.

I'm not arguing for or a against, you just need a test that enforces the current behavior.

as noted in PR msg, this is on TODO list but could be done in subsequent PR while being backward compatible

Same here, test.

jacob-carlborg · 2018-01-31T10:24:35Z

I don't want to completely ruin this PR, but what about a __traits instead of this magic template or whatever it is?

thewilsonator · 2018-01-31T14:13:30Z

src/dmd/tokens.d

    pound,

-    // 239
+    // 241


You only added one token, why does the count go up by two?

IIRC fileFullPath had been added without upading these numbers in the past, so i also correct that; however I'd super like to just get rid of these numbers, they serve no purpose and get out of sync easily; shd be done in a separate PR though; ok to remove in another PR?

Fair enough, just making sure you hadn't typo'd. I think they have some use when trying to look up what Tok you have in some function but I'll leave it up to other to determine what to do.

I see; in this case one could use instead arbitrary increasing numbers at sparse intervals, it's much more robust (and more stable across different compiler versions)

enum TOK : int { reserved = 0, // we can reset numbers arbitrarily leftParentheses = 100, rightParentheses, // will be 101count) leftBracket = 200, }

thewilsonator · 2018-01-31T14:15:33Z

How does this work with varadic parameters? what about getting stringification of multiple parameters in a string[]?

rainers · 2018-02-01T06:59:43Z

what about a __traits instead

There's even parameters reserved as a __traits parameter, but isn't filled with functionality: https://run.dlang.io/gist/d9ab0db568456f102a1e39af574eae4c?compiler=dmd

timotheecour · 2018-02-01T09:44:28Z

I don't want to completely ruin this PR, but what about a __traits instead of this magic template or whatever it is?

after discussion on slack, the only difference with proposed PR would be a minor syntactic change (__ARG_STRING_!a vs __traits(argString, a)); I've discussed pros/cons in #7821 (comment) ; could switch to that if there's consensus

NOTE: I'm closing in favor of #7821 which proposes a more robust implementation that avoids technical issues with not having source code lying around, eg FILE and enum's got folded too early

timotheecour requested review from dnadlinger and ibuclaw as code owners January 30, 2018 22:10

dlang-bot added the Review:WIP Work In Progress - not ready for review or pulling label Jan 30, 2018

timotheecour force-pushed the pr_arg_names branch from 06c384c to a246ded Compare January 31, 2018 00:30

timotheecour changed the title ~~[WIP] function argument stringification: __ARG_NAME__~~ [WIP] function argument stringification: __ARG_STRING__ Jan 31, 2018

jacob-carlborg suggested changes Jan 31, 2018

View reviewed changes

timotheecour force-pushed the pr_arg_names branch from a246ded to be57dc5 Compare January 31, 2018 08:01

timotheecour requested review from MartinNowak and wilzbach as code owners January 31, 2018 08:01

dlang-bot added the Severity:Bug Fix label Jan 31, 2018

jacob-carlborg suggested changes Jan 31, 2018

View reviewed changes

timotheecour added 7 commits January 31, 2018 00:34

function argument stringification: __ARG_STRING__

398165e

only set stringified as needed in callMatch, depending on isArgString…

11ed415

…ify computed from candidate matching function

cleanup

32ac8ef

cleanup

06255f9

cleanup

20896ff

cleanup

ddc547e

cleanup

90fbb28

timotheecour force-pushed the pr_arg_names branch from 49e2a9d to 90fbb28 Compare January 31, 2018 08:34

cleanup

a74a9ef

added test cases with Expressions with user defined types and More co…

fbb5287

…mplex expressions

thewilsonator reviewed Jan 31, 2018

View reviewed changes

timotheecour mentioned this pull request Feb 1, 2018

function argument stringification: __traits(getCallerSource, symbol) (variadics work too) #7821

Closed

timotheecour changed the title ~~[WIP] function argument stringification: __ARG_STRING__~~ [WIP] function argument stringification: __ARG_STRING__ (replaced by 7821) Feb 1, 2018

timotheecour closed this Feb 1, 2018

                   pound,
-                  // 239
+                  // 241

Uh oh!

Conversation

timotheecour commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Uh oh!

wilzbach commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timotheecour commented Jan 31, 2018

Uh oh!

jacob-carlborg commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg commented Jan 31, 2018

Uh oh!

jacob-carlborg left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg commented Jan 31, 2018

Uh oh!

timotheecour commented Jan 31, 2018

Uh oh!

timotheecour commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacob-carlborg commented Jan 31, 2018

Uh oh!

jacob-carlborg commented Jan 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timotheecour Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thewilsonator commented Jan 31, 2018

Uh oh!

rainers commented Feb 1, 2018

Uh oh!

timotheecour commented Jan 30, 2018 •

edited

Loading

dlang-bot commented Jan 30, 2018 •

edited

Loading

wilzbach commented Jan 30, 2018 •

edited

Loading

jacob-carlborg commented Jan 31, 2018 •

edited

Loading

jacob-carlborg left a comment •

edited

Loading

timotheecour commented Jan 31, 2018 •

edited

Loading

timotheecour Feb 1, 2018 •

edited

Loading

timotheecour commented Feb 1, 2018 •

edited

Loading