Skip to content

mir.ndslice: improve codestyle #154

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
9il merged 2 commits into from
Apr 18, 2016
Merged

mir.ndslice: improve codestyle #154

9il merged 2 commits into from
Apr 18, 2016

Conversation

@wilzbach
Copy link
Member

@wilzbach wilzbach commented Apr 18, 2016

I still had these small improvement laying around - with them I can enable dscanner for mir.ndslice as it now passes our code style.

It's mostly just > 120 lines, missing const, better readability for numbers and undocumented methods.

wilzbach
wilzbach reviewed Apr 18, 2016
View reviewed changes
source/mir/ndslice/selection.d
// undocumented
// zero cost variant of `std.range.iota`
///
struct IotaMap()
Copy link
Member Author

@wilzbach wilzbach Apr 18, 2016

Choose a reason for hiding this comment

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

I don't remember exactly, but you wanted to document it instead of setting it to private or protected?

Copy link
Member

@9il 9il Apr 18, 2016

Choose a reason for hiding this comment

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

just undocumented

Copy link
Member Author

@wilzbach wilzbach Apr 18, 2016

Choose a reason for hiding this comment

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

Dscanner rightfully complains about that as you shouldn't have anything that is public and undocumented...

Copy link
Member

@9il 9il Apr 18, 2016

Choose a reason for hiding this comment

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

Turn it off)

@wilzbach wilzbach force-pushed the improve_codestyle2 branch from f1a255e to 378579d Compare April 18, 2016 09:37
@codecov-io
Copy link

codecov-io commented Apr 18, 2016

Current coverage is 98.55%

Merging #154 into master will not affect coverage as of 919ef48

@@            master    #154   diff @@
======================================
  Files            7       7       
  Stmts         2636    2637     +1
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           2598    2599     +1
  Partial          0       0       
  Missed          38      38

Review entire Coverage Diff as of 919ef48

Uncovered Suggestions

  1. +0.23% via ...ir/ndslice/package.d#359...364
  2. +0.23% via source/mir/las/sum.d#757...762
  3. +0.19% via .../mir/ndslice/slice.d#2820...2824
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

@wilzbach wilzbach force-pushed the improve_codestyle2 branch 2 times, most recently from 9254e25 to 5058cb1 Compare April 18, 2016 10:09
@wilzbach wilzbach mentioned this pull request Apr 18, 2016
Closed
@wilzbach
Copy link
Member Author

wilzbach commented Apr 18, 2016

Turn it off)

Imho it is one of the more useful switches as I found > 10 occurrences in the codebase of undocumented public methods. So I would like to keep that flag.
Can't we set it to protected or private as long as you don't want to expose IotaMap to the user?

@9il
Copy link
Member

9il commented Apr 18, 2016

Can't we set it to protected or private as long as you don't want to expose IotaMap to the user?

OK, document it)

@wilzbach wilzbach force-pushed the improve_codestyle2 branch 2 times, most recently from 8d8c32a to 9f47784 Compare April 18, 2016 12:26
@wilzbach
Copy link
Member Author

wilzbach commented Apr 18, 2016

OK, document it)

like this or with more text?

@9il
Copy link
Member

9il commented Apr 18, 2016

like this or with more text?

few words

9il
9il reviewed Apr 18, 2016
View reviewed changes
source/mir/ndslice/selection.d Outdated
// undocumented
// zero cost variant of `std.range.iota`
/++
Zero cost variant of `std.range.iota`
Copy link
Member

@9il 9il Apr 18, 2016

Choose a reason for hiding this comment

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

Zero cost variant of std.range.iota - this should be with // . Just add that this range is used for iotaSlice.

Copy link
Member

@9il 9il Apr 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, left it, but with small change:

Zero cost variant of `std.range.iota` for iotaSlice.

And make std.range.iota a link to iota

@wilzbach wilzbach force-pushed the improve_codestyle2 branch from 9f47784 to 88bb7b4 Compare April 18, 2016 12:38
@wilzbach
Copy link
Member Author

wilzbach commented Apr 18, 2016

updated - i used the new REF_ALTTEXT macro , which doesn't work yet with bootDoc, but works for Phobos.

@9il
Copy link
Member

9il commented Apr 18, 2016

REF_ALTTEXT

What is this and would it work for Phobos?

@wilzbach
Copy link
Member Author

wilzbach commented Apr 18, 2016

What is this and would it work for Phobos?

Yep I recently found it dlang/phobos#4079 - i think it was part of a move to deprecate a lot of old macros within Phobos, but this PR stalled somehow.
It allows to have an alternative text for references - do you know a better macro?

@9il
Copy link
Member

9il commented Apr 18, 2016

It allows to have an alternative text for references - do you know a better macro?
Hide all checks

no

@9il 9il merged commit e62ff72 into libmir:master Apr 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@wilzbach @codecov-io @9il