deduplicate the 'isSomeFunction' equivalent by UplinkCoder · Pull Request #6835 · dlang/dmd

UplinkCoder · 2017-05-27T10:55:35Z

Since the code is now used multiple times in __traits let's dedup it.

rainers · 2017-05-27T11:08:39Z

src/ddmd/traits.d

        return new IntegerExp(e.loc, false, Type.tbool);
    }

+    TypeFunction firstArgFunctionType()


IMO a better abstraction is toTypeFunction(RootObject o) and passing the validated parameter into this function. That would allow it not being a nested function and used elsewhere, too.

Even better, I'd split it into two overloads, one for Type, and one for Dsymbol, as you now have widen the allowed parameters to traits that support types only so far.

the reason I did not do this is because I'd have to import rootobject

also let's generalize when/if needed and not on speculation.

@rainers do you agree ?

do you agree ?

Mostly. The diff didn't show that there's also support for function symbols in the other traits.

You should remove the handling for FuncDeclaration in traits getLinkage, getFunctionVariadicStyle and getParameterStorageClasses, though. It is duplicated now.

the reason I did not do this is because I'd have to import rootobject

That's not really a good excuse. You can also use typeof(args[0]) to avoid an import... ;-)

UplinkCoder · 2017-05-27T11:24:30Z

travis fails because of connection error

wilzbach · 2017-05-27T11:43:32Z

@UplinkCoder: restarted and created another card for it https://trello.com/c/aM5r3Bio/320-use-more-download-fallbacks-in-the-installer-script

UplinkCoder · 2017-05-27T19:06:11Z

@rainers do you like it better now ?

UplinkCoder · 2017-05-27T19:36:31Z

@WalterBright any objections ?

WalterBright · 2017-05-28T02:03:47Z

src/ddmd/traits.d

        return new IntegerExp(e.loc, false, Type.tbool);
    }

+    static TypeFunction traitsFuncArg(RootObject o, FuncDeclaration* fdp = null)


No documentation saying what function does.

WalterBright · 2017-05-28T02:06:26Z

LGTM provided the function is documented

WalterBright · 2017-05-28T02:15:57Z

use typeof(args[0]) to avoid an import... ;-)

Not a bad idea.

WalterBright · 2017-05-28T21:15:49Z

src/ddmd/traits.d

+     In case of a function declaration being passed verbatim
+     it will set the second  optional argument to the FuncDeclarartion
+     given it was provided.
+    */


Please use Ddoc style documentation. It this case it means having a Params: section and a Returns: section. This is the standard for new functions.

@UplinkCoder don't forget that we already publish the documentation of ddmd on dlang.org:

http://dlang.org/phobos-prerelease/ddmd_mars.html (Ddoc)

http://dlang.org/library-prerelease/ddmd/mars.html (Ddox)
and we intend to publish it on the main page very soon (e.g. once Fix issue 17392 - Add Dub file for the lexer and parser #6771 is merged).

This is also why it can be very helpful for both the contributor and the reviewers to preview the style changes at Vladimir's excellent DAutoTest, e.g.

http://dtest.dlang.io/artifact/website-72ba79b73f635fb08bdfe69d6826833c1c4c2ee0-6f020744a26d63533038dc39e44ae752/web/library-prerelease/ddmd/traits.html

(in this case it's a function within a function and thus not publicly accessible)

WalterBright · 2017-05-28T21:16:13Z

src/ddmd/traits.d


+    /**
+     if traitsFuncArg is passed either a delegate. a function
+     or a function pointer or a variable of types.


omit trailing period in middle of sentence

WalterBright · 2017-05-28T21:16:59Z

src/ddmd/traits.d

+     it will return the functionType.
+     else it will return null
+
+     In case of a function declaration being passed verbatim


passed verbatim has no meaning in regards to code.

WalterBright · 2017-05-29T19:34:45Z

src/ddmd/traits.d

+     Returns: a function-type if
+        o is a declarion of type  delegate, function,
+            or function pointer
+        or a variable of those types.


When using periods, please do not put them in the middle of sentences. Replace it with a ,

@WalterBright do you like it better now ?

UplinkCoder · 2017-05-29T22:21:29Z

@WalterBright addressed.

WalterBright · 2017-05-29T23:49:23Z

src/ddmd/traits.d

    }

+    /**
+     Params: o: the AST-node to check.


Please read the Ddoc documentation I provided a link for. Note the o: in the above line. This is not how it's done and Ddoc will not work with it. It's o=.

JinShil · 2017-12-19T00:35:13Z

@UplinkCoder Are you going to take this across the finish line?

JinShil · 2017-12-19T00:37:28Z

src/ddmd/traits.d


+    /**
+     Params: o: the AST-node to check.
+     Params: fdp: optionally a pointer to a function declararion,


declararion --> declaration

JinShil · 2017-12-19T00:43:30Z

src/ddmd/traits.d

+     else
+        it will return null.
+    */
+    static TypeFunction traitsFuncArg(RootObject o, FuncDeclaration* fdp = null)


I recommend a different name. Perhaps getTypeFunction.

JinShil · 2017-12-19T00:54:12Z

src/ddmd/traits.d

+        it will return the functionType
+     else
+        it will return null.
+    */


I offer you this documentation header:

/** Gets the `TypeFunction` for `o` Params: o = the AST node to get the `TypeFunction` for fdp = optional pointer to a function declararion, to be set if `o` is a function declarartion Returns: a `TypeFunction` if `o` is a declaration for a delegate, function, or avariable of those types. Otherwise, `null` */

Neat Thanks!

andralex · 2017-12-20T18:02:14Z

@UplinkCoder ping?

dlang-bot · 2017-12-21T08:36:02Z

Thanks for your pull request, @UplinkCoder!

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.

JinShil · 2017-12-21T09:42:35Z

src/dmd/traits.d

+          or a variable of the former.  Otherwise, `null`.
+    */
+
+    static TypeFunction traitsFuncArg(RootObject o, FuncDeclaration* fdp = null)


All other DDoc headers in DMD seem to follow this convention:

/************************************************************* * Whatever * Whatever */

Please remove the extra space between the DDoc header and the function declaration.

I cannot, in good conscience, approve a function name like traitsFuncArg. Please use a more descriptive name like getFuncType or something along those lines (Yes, I know my own names have been poor).

Please remove the extra space between the DDoc header and the function declaration.

Hmm the above is the Phobos style:
Compare with e.g. https://github.com/dlang/phobos/blob/master/std/algorithm/comparison.d

If you really want to stick with this style at DMD, then this should be documented in a DMD-specific style guide?

Hmm the above is the Phobos style.

I see.

If you really want to stick with this style at DMD, then this should be documented in a DMD-specific style guide.

Agreed.

Aside from the name then, LGTM.

Also, consider making this a non-member function instead of static, and preferably private. Refer to @WalterBright's comment

Phobos has a mixed style when it comes to comments [1] [2] [3] [4]. This is even within the same module.

[1] https://github.com/dlang/phobos/blob/master/std/string.d#L2194-L2205
[2] https://github.com/dlang/phobos/blob/master/std/string.d#L2254-L2267
[3] https://github.com/dlang/phobos/blob/master/std/string.d#L2332-L2358
[4] https://github.com/dlang/phobos/blob/master/std/string.d#L2664-L2689

Let's use https://github.com/dlang/phobos/blob/master/std/string.d#L2254-L2267 for new code, thanks.

JinShil · 2018-01-17T01:54:30Z

This PR needs to have the reviewers' concerns addressed and rebased.

This PR is nice, and normally I would revive such a PR if the author was no longer active. I would also revive it if it were an important bug fix or something of higher priority. This PR, however, is just a refactoring, so I'm going to put it in the Phantom Zone and close it for now.

JinShil · 2018-01-18T23:52:10Z

Resubmitted at #7749

rainers reviewed May 27, 2017

View reviewed changes

UplinkCoder force-pushed the function_traits branch 2 times, most recently from 722b1f9 to 58d5861 Compare May 27, 2017 19:04

UplinkCoder force-pushed the function_traits branch from 58d5861 to ffe5e23 Compare May 27, 2017 19:18

WalterBright reviewed May 28, 2017

View reviewed changes

UplinkCoder force-pushed the function_traits branch 3 times, most recently from 41897b0 to 3b8ef98 Compare May 28, 2017 11:38

WalterBright reviewed May 28, 2017

View reviewed changes

WalterBright reviewed May 29, 2017

View reviewed changes

UplinkCoder force-pushed the function_traits branch from 47a68fb to 61c95c9 Compare May 29, 2017 22:21

WalterBright reviewed May 29, 2017

View reviewed changes

JinShil reviewed Dec 19, 2017

View reviewed changes

JinShil suggested changes Dec 19, 2017

View reviewed changes

UplinkCoder force-pushed the function_traits branch from 61c95c9 to 497cd06 Compare December 21, 2017 08:36

deduplicate the 'isSomeFunction' equivalent

00977b0

UplinkCoder force-pushed the function_traits branch from 497cd06 to 00977b0 Compare December 21, 2017 08:48

JinShil suggested changes Dec 21, 2017

View reviewed changes

dlang-bot added Review:Needs Rebase Review:Needs Work and removed Review:Needs Rebase labels Jan 1, 2018

dlang-bot added the Review:Needs Rebase label Jan 16, 2018

JinShil added the Review:Phantom Zone Has value/information for future work, but closed for now label Jan 17, 2018

JinShil closed this Jan 17, 2018

JinShil added the Severity:Refactoring No semantic changes to code label Jan 17, 2018

JinShil mentioned this pull request Jan 18, 2018

deduplicate the 'isSomeFunction' equivalent #7749

Merged

JinShil removed the Review:Phantom Zone Has value/information for future work, but closed for now label Jan 18, 2018

JinShil mentioned this pull request Jan 19, 2018

some stylistic improvements to toTypeFunction() #7754

Merged

JinShil mentioned this pull request Jan 26, 2018

Improve destroy's documentation dlang/druntime#2054

Merged

Uh oh!

Conversation

UplinkCoder commented May 27, 2017

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!

UplinkCoder commented May 27, 2017

Uh oh!

wilzbach commented May 27, 2017

Uh oh!

UplinkCoder commented May 27, 2017

Uh oh!

UplinkCoder commented May 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WalterBright commented May 28, 2017

Uh oh!

WalterBright commented May 28, 2017

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!

UplinkCoder commented May 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JinShil commented Dec 19, 2017

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!

andralex commented Dec 20, 2017

Uh oh!

dlang-bot commented Dec 21, 2017

Bugzilla references

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!

JinShil Dec 21, 2017 • 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!

jacob-carlborg Dec 21, 2017 • 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!

JinShil commented Jan 17, 2018

Uh oh!

JinShil commented Jan 18, 2018

Uh oh!

Reviewers

JinShil Dec 21, 2017 •

edited

Loading

jacob-carlborg Dec 21, 2017 •

edited

Loading