Skip to content
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.

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

[ndslice] Math Index Order #3940

Merged
merged 2 commits into from
Mar 25, 2016
Merged

[ndslice] Math Index Order #3940

merged 2 commits into from
Mar 25, 2016

Conversation

9il
Copy link
Member

@9il 9il commented Jan 20, 2016

@9il 9il mentioned this pull request Jan 20, 2016
@@ -57,6 +57,12 @@ Multidimensional view is presented by $(SUBREF slice, Slice) type.
------
auto matrix = new double[12].sliced(3, 4);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this is a good opportunity to move this to a unit test example?

Copy link
Member Author

Choose a reason for hiding this comment

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

No

Copy link
Member

Choose a reason for hiding this comment

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

Why not?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. I want this example be in the beginning
  2. DDOC unittest do not work with module declarations
  3. The same unittest already exists without DDOC

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

There is now a fix for this, that will allow (finally!!!) ddoc's unittests to work with module declarations: dlang/dmd#5465

Copy link
Member

Choose a reason for hiding this comment

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

FYI, dlang/dmd#5465 has been merged, so you can now use ddoc'd unittests for the module docs.

@timotheecour
Copy link
Contributor

Why not simply:

  auto ref opCall(Indexes...)(Indexes _indexes){
    import std.meta;
    return opIndex(Reverse!_indexes);
  }

there should be no runtime penalty in doing so

@9il
Copy link
Member Author

9il commented Jan 21, 2016

there should be no runtime penalty in doing so

Yes, but

  1. Error message should be another
  2. Penalty if index is defined as static array
  3. This will work a little bit faster in mode without inlining (for indexing it is matter)

@timotheecour
Copy link
Contributor

  1. Error message should be another

This is easy to address: simply passing along a CT parameter bool isMathMode to tell how to format error message

@9il
Copy link
Member Author

9il commented Jan 21, 2016

This is easy to address: simply passing along a CT parameter bool isMathMode to tell how to format error message

pragma(msg, (5-2).stringof); prints 5-2. So, an additional template should be defined :
Code is already small and clear after message refactoring.

@timotheecour
Copy link
Contributor

will it generalize to arbitrary indexing situations without additional code duplication?
(beyond scalar indexes: I'm thinking about this: libmir/mir#10)

the advantage of opIndex(Reverse!_indexes) is that it would work in all indexing situations.

Another advantage could be more code reuse, therefore smaller generated code and less instruction cache miss.

pragma(msg, (5-2).stringof); prints 5-2. So, an additional template should be defined :\

sorry I don't understand what you mean here

@9il
Copy link
Member Author

9il commented Jan 21, 2016

will it generalize to arbitrary indexing situations without additional code duplication?

According to the index type definitions

  1. math fully defined indexes are implemented in this PR
  2. math fully defined slices can be implemented in the future with something like allow strided slice indexing: m[0, 0..5, _, R(0, $, 2), R($-1, 0, -2) ] libmir/mir#10
  3. math partially defined slices are not defined

fully defined slices can be implemented without code duplication.

@9il
Copy link
Member Author

9il commented Jan 21, 2016

... continue

the advantage of opIndex(Reverse!_indexes) is that it would work in all indexing situations.

Yes (excluding static arrays), I don't want it for now.

Another advantage could be more code reuse, therefore smaller generated code and less instruction cache miss.

The amount of code in the release mode is obviously the same (because inlining).
Code duplication will be faster for debug mode.

To make this code reusable I need to write additional templates because opIndex(Reverse!_indexes) would not work for static arrays. After all, amount of lines of code would be bigger with generic solution.

@9il
Copy link
Member Author

9il commented Jan 21, 2016

In addition, this function declaration is good for documentation.

@9il 9il changed the title Math Index Order [ndslice] Math Index Order Feb 22, 2016
@quickfur
Copy link
Member

I trust that @9il knows what he's doing with the ndslice module, plus this is still under experimental, so I'm merging this. Nitpicks can be fixed later.

@9il
Copy link
Member Author

9il commented Mar 24, 2016

Thx!

@quickfur
Copy link
Member

Auto-merge toggled on

@quickfur quickfur merged commit 3fb9f54 into dlang:master Mar 25, 2016
@9il 9il added the ndslice label Apr 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants