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

fix evaluating one/zero in generator body #8

Merged
merged 1 commit into from
Feb 27, 2017
Merged

Conversation

KristofferC
Copy link
Collaborator

This should fix #7 cc @andyferris

@codecov-io
Copy link

Codecov Report

Merging #8 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master      #8      +/-   ##
=========================================
+ Coverage   96.59%   96.6%   +0.01%     
=========================================
  Files          13      13              
  Lines         675     677       +2     
=========================================
+ Hits          652     654       +2     
  Misses         23      23
Impacted Files Coverage Δ
src/constructors.jl 98.14% <100%> (+0.07%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49d600a...1454860. Read the comment docs.

@fredrikekre fredrikekre self-requested a review February 27, 2017 18:21
@KristofferC
Copy link
Collaborator Author

KristofferC commented Feb 27, 2017

Heh.. was going to put you on review and you did it yourself 17 sec ago... Same wavelength brooo.

@KristofferC KristofferC merged commit aa4fd9b into master Feb 27, 2017
@KristofferC KristofferC deleted the kc/worlds branch February 27, 2017 18:23
@andyferris
Copy link

I didn't look over the all the codebase - is this the only place that this kind of thing happens? Does it solve the using vs include error you saw?

@fredrikekre
Copy link
Member

Does it solve the using vs include error you saw?

No.

@KristofferC
Copy link
Collaborator Author

KristofferC commented Feb 27, 2017

Problem is here https://github.com/KristofferC/Tensors.jl/blob/49d600a05c07407f410bdf74ebe9eae444748929/src/constructors.jl#L42

I guess it is the same if the type is defined after that function (get_type).

@andyferris
Copy link

Yeah, I saw that and it definitely raised some red flags in my mind, but I didn't exactly know what they were. It seemed a bit like the similar_type problem in StaticArrays (similar_type is the counterpart to similar for immutable arrays).

@KristofferC
Copy link
Collaborator Author

But it is weird because the Tensor type is defined at that point. We don't expect anyone to subtype these types so we should have it easier than StaticArrays. Anyway, I'm not sure what the difference is between first include the module file and then using it ... Perhaps the @generated stuff runs but the types are still not really defined... I dunno. :/

@andyferris
Copy link

Unlike most of Julia code, the order of generated functions w.r.t. other method definitions is important, even if they are simultaneously loaded from the same file. Maybe shuffling the order of stuff could help.

Not sure why using vs include should cause any difference there, however... Seems a mystery to me.

@fredrikekre
Copy link
Member

Maybe shuffling the order of stuff could help.

Ref #9

Thanks for the tip!

@andyferris
Copy link

No worries. :)

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.

@generated function generators don't follow the new rules for v0.6
4 participants