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

Implemented yielding Dir.each_child #4811

Merged
merged 6 commits into from Aug 13, 2017

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Aug 8, 2017

Complements #4808

@Sija
Copy link
Contributor Author

Sija commented Aug 8, 2017

Ooops, build on 32-bit linux box failed with sigfault:

Invalid memory access (signal 11) at address 0x25eeb4d0

[0x902449a] *CallStack::print_backtrace:Int32 +154
[0x893900f] __crystal_sigfault_handler +111
[0xbc4f3f1] sigfault_handler +37
[0xf77b0cc0] ???
[0xbb5a572] _ZN4llvm23ReplaceableMetadataImpl7dropRefEPv +50
[0xbb5a65c] _ZN4llvm16MetadataTracking7untrackEPvRNS_8MetadataE +44
[0xbb5dd90] _ZN4llvm6MDNode10setOperandEjPNS_8MetadataE +64
[0xbb5dfcf] _ZN4llvm6MDNode17dropAllReferencesEv +47
[0xbb4644d] _ZN4llvm15LLVMContextImplD1Ev +11389
[0xbb3e8c3] _ZN4llvm11LLVMContextD1Ev +35
[0xba59fd1] LLVMContextDispose +33
[0xa55a398] *LLVM::Context#finalize:Nil +184
[0x8946b9e] ~proc7Proc(Pointer(Void), Pointer(Void), Nil) +30
[0xbc3edce] GC_invoke_finalizers +110
[0xbc3f036] GC_notify_or_invoke_finalizers +198
[0xbc40a51] GC_generic_malloc_many +81
[0xbc492ed] GC_malloc +205
[0x891b5ce] __crystal_malloc +30
[0x9e814f5] *Crystal::NumberLiteral::new<String, Symbol>:Crystal::NumberLiteral +53
[0xa2f4297] *Crystal::Parser#parse_atomic_without_location:Crystal::ASTNode+ +1159
[0xa2f3d73] *Crystal::Parser#parse_atomic:Crystal::ASTNode+ +67
[0xa2f3d03] *Crystal::Parser#parse_atomic_with_method:Crystal::ASTNode+ +67
[0xa2f3837] *Crystal::Parser#parse_prefix:Crystal::ASTNode+ +215
[0xa2f3556] *Crystal::Parser#parse_pow:Crystal::ASTNode+ +70
[0xa2f325c] *Crystal::Parser#parse_mul_or_div:Crystal::ASTNode+ +76
[0xa2f2bdf] *Crystal::Parser#parse_add_or_sub:Crystal::ASTNode+ +95
[0xa2f2936] *Crystal::Parser#parse_shift:Crystal::ASTNode+ +70
[0xa2f26e6] *Crystal::Parser#parse_logical_and:Crystal::ASTNode+ +70
[0xa2f2456] *Crystal::Parser#parse_logical_or:Crystal::ASTNode+ +70
[0xa2f207c] *Crystal::Parser#parse_cmp:Crystal::ASTNode+ +76
[0xa2f1c9c] *Crystal::Parser#parse_equality:Crystal::ASTNode+ +76
[0xa2f1af3] *Crystal::Parser#parse_and:Crystal::ASTNode+ +67
[0xa2f1953] *Crystal::Parser#parse_or:Crystal::ASTNode+ +67
[0xa2f1824] *Crystal::Parser#parse_range:Crystal::ASTNode+ +68
[0xa2f169c] *Crystal::Parser#parse_question_colon:Crystal::ASTNode+ +44
[0xa2f0281] *Crystal::Parser#parse_op_assign<Bool, Bool>:Crystal::ASTNode+ +193
[0xa30706c] *Crystal::Parser#parse_op_assign_no_control<Bool, Bool>:Crystal::ASTNode+ +92
[0xa306ffc] *Crystal::Parser#parse_op_assign_no_control:Crystal::ASTNode+ +76
[0xa330f0d] *Crystal::Parser#parse_call_arg:Crystal::ASTNode+ +317
[0xa317b3c] *Crystal::Parser#parse_call_args<Bool, Bool, Bool>:(Crystal::Parser::CallArgs | Nil) +1724
[0xa3173ca] *Crystal::Parser#parse_call_args:stop_on_do_after_space<Bool>:(Crystal::Parser::CallArgs | Nil) +122
[0xa315035] *Crystal::Parser#parse_var_or_call<Bool, Bool>:Crystal::ASTNode+ +1701
[0xa344ddc] *Crystal::Parser#parse_var_or_call:Crystal::ASTNode+ +76
[0xa2facc4] *Crystal::Parser#parse_atomic_without_location:Crystal::ASTNode+ +28340
[0xa2f3d73] *Crystal::Parser#parse_atomic:Crystal::ASTNode+ +67
[0xa2f3d03] *Crystal::Parser#parse_atomic_with_method:Crystal::ASTNode+ +67
[0xa2f3837] *Crystal::Parser#parse_prefix:Crystal::ASTNode+ +215
[0xa2f3556] *Crystal::Parser#parse_pow:Crystal::ASTNode+ +70
[0xa2f325c] *Crystal::Parser#parse_mul_or_div:Crystal::ASTNode+ +76
[0xa2f2bdf] *Crystal::Parser#parse_add_or_sub:Crystal::ASTNode+ +95
[0xa2f2936] *Crystal::Parser#parse_shift:Crystal::ASTNode+ +70
[0xa2f26e6] *Crystal::Parser#parse_logical_and:Crystal::ASTNode+ +70
[0xa2f2456] *Crystal::Parser#parse_logical_or:Crystal::ASTNode+ +70
[0xa2f207c] *Crystal::Parser#parse_cmp:Crystal::ASTNode+ +76
[0xa2f1c9c] *Crystal::Parser#parse_equality:Crystal::ASTNode+ +76
[0xa2f1af3] *Crystal::Parser#parse_and:Crystal::ASTNode+ +67
[0xa2f1953] *Crystal::Parser#parse_or:Crystal::ASTNode+ +67
[0xa2f1824] *Crystal::Parser#parse_range:Crystal::ASTNode+ +68
[0xa2f169c] *Crystal::Parser#parse_question_colon:Crystal::ASTNode+ +44
[0xa2f0281] *Crystal::Parser#parse_op_assign<Bool, Bool>:Crystal::ASTNode+ +193
[0xa2f01ac] *Crystal::Parser#parse_op_assign:Crystal::ASTNode+ +76
[0xa2f0133] *Crystal::Parser#parse_expression:Crystal::ASTNode+ +67
[0xa2ef6d0] *Crystal::Parser#parse_multi_assign:Crystal::ASTNode+ +96
[0xa2ef008] *Crystal::Parser#parse_expressions_internal:Crystal::ASTNode+ +248
[0xa2eeeeb] *Crystal::Parser#parse_expressions:Crystal::ASTNode+ +75
[0xa333e96] *Crystal::Parser#parse_begin:Crystal::ASTNode+ +86
[0xa2f5283] *Crystal::Parser#parse_atomic_without_location:Crystal::ASTNode+ +5235
[0xa2f3d73] *Crystal::Parser#parse_atomic:Crystal::ASTNode+ +67
[0xa2f3d03] *Crystal::Parser#parse_atomic_with_method:Crystal::ASTNode+ +67
[0xa2f3837] *Crystal::Parser#parse_prefix:Crystal::ASTNode+ +215
[0xa2f3556] *Crystal::Parser#parse_pow:Crystal::ASTNode+ +70
[0xa2f325c] *Crystal::Parser#parse_mul_or_div:Crystal::ASTNode+ +76
[0xa2f2bdf] *Crystal::Parser#parse_add_or_sub:Crystal::ASTNode+ +95
[0xa2f2936] *Crystal::Parser#parse_shift:Crystal::ASTNode+ +70
[0xa2f26e6] *Crystal::Parser#parse_logical_and:Crystal::ASTNode+ +70
[0xa2f2456] *Crystal::Parser#parse_logical_or:Crystal::ASTNode+ +70
[0xa2f207c] *Crystal::Parser#parse_cmp:Crystal::ASTNode+ +76
[0xa2f1c9c] *Crystal::Parser#parse_equality:Crystal::ASTNode+ +76
[0xa2f1af3] *Crystal::Parser#parse_and:Crystal::ASTNode+ +67
[0xa2f1953] *Crystal::Parser#parse_or:Crystal::ASTNode+ +67
[0xa2f1824] *Crystal::Parser#parse_range:Crystal::ASTNode+ +68
[0xa2f169c] *Crystal::Parser#parse_question_colon:Crystal::ASTNode+ +44
[0xa2f0281] *Crystal::Parser#parse_op_assign<Bool, Bool>:Crystal::ASTNode+ +193
[0xa30706c] *Crystal::Parser#parse_op_assign_no_control<Bool, Bool>:Crystal::ASTNode+ +92
[0xa306ffc] *Crystal::Parser#parse_op_assign_no_control:Crystal::ASTNode+ +76
[0xa2f15c1] *Crystal::Parser#parse_op_assign<Bool, Bool>:Crystal::ASTNode+ +5121
[0xa2f01ac] *Crystal::Parser#parse_op_assign:Crystal::ASTNode+ +76
[0xa2f0133] *Crystal::Parser#parse_expression:Crystal::ASTNode+ +67
[0xa2ef6d0] *Crystal::Parser#parse_multi_assign:Crystal::ASTNode+ +96
[0xa2eef60] *Crystal::Parser#parse_expressions_internal:Crystal::ASTNode+ +80
[0xa2eeeeb] *Crystal::Parser#parse_expressions:Crystal::ASTNode+ +75
[0xa33731e] *Crystal::Parser#parse_def_helper<Bool, Bool>:Crystal::Def +6462
[0xa33aa46] *Crystal::Parser#parse_def<Bool, Bool, Nil>:Crystal::Def +134
[0xa33a9ac] *Crystal::Parser#parse_def:Crystal::Def +76
[0xa2f688d] *Crystal::Parser#parse_atomic_without_location:Crystal::ASTNode+ +10877
[0xa2f3d73] *Crystal::Parser#parse_atomic:Crystal::ASTNode+ +67
[0xa2f3d03] *Crystal::Parser#parse_atomic_with_method:Crystal::ASTNode+ +67
[0xa2f3837] *Crystal::Parser#parse_prefix:Crystal::ASTNode+ +215
[0xa2f3556] *Crystal::Parser#parse_pow:Crystal::ASTNode+ +70
[0xa2f325c] *Crystal::Parser#parse_mul_or_div:Crystal::ASTNode+ +76
[0xa2f2bdf] *Crystal::Parser#parse_add_or_sub:Crystal::ASTNode+ +95
[0xa2f2936] *Crystal::Parser#parse_shift:Crystal::ASTNode+ +70
[0xa2f26e6] *Crystal::Parser#parse_logical_and:Crystal::ASTNode+ +70
[0xa2f2456] *Crystal::Parser#parse_logical_or:Crystal::ASTNode+ +70
[0xa2f207c] *Crystal::Parser#parse_cmp:Crystal::ASTNode+ +76
[0xa2f1c9c] *Crystal::Parser#parse_equality:Crystal::ASTNode+ +76
[0xa2f1af3] *Crystal::Parser#parse_and:Crystal::ASTNode+ +67
[0xa2f1953] *Crystal::Parser#parse_or:Crystal::ASTNode+ +67
[0xa2f1824] *Crystal::Parser#parse_range:Crystal::ASTNode+ +68
[0xa2f169c] *Crystal::Parser#parse_question_colon:Crystal::ASTNode+ +44
[0xa2f0281] *Crystal::Parser#parse_op_assign<Bool, Bool>:Crystal::ASTNode+ +193
[0xa2f01ac] *Crystal::Parser#parse_op_assign:Crystal::ASTNode+ +76
[0xa34406e] *Crystal::Parser#parse_visibility_modifier<Crystal::Visibility>:Crystal::VisibilityModifier +190
[0xa2fa881] *Crystal::Parser#parse_atomic_without_location:Crystal::ASTNode+ +27249
[0xa2f3d73] *Crystal::Parser#parse_atomic:Crystal::ASTNode+ +67
[0xa2f3d03] *Crystal::Parser#parse_atomic_with_method:Crystal::ASTNode+ +67
[0xa2f3837] *Crystal::Parser#parse_prefix:Crystal::ASTNode+ +215
[0xa2f3556] *Crystal::Parser#parse_pow:Crystal::ASTNode+ +70
[0xa2f325c] *Crystal::Parser#parse_mul_or_div:Crystal::ASTNode+ +76
[0xa2f2bdf] *Crystal::Parser#parse_add_or_sub:Crystal::ASTNode+ +95
[0xa2f2936] *Crystal::Parser#parse_shift:Crystal::ASTNode+ +70
[0xa2f26e6] *Crystal::Parser#parse_logical_and:Crystal::ASTNode+ +70
[0xa2f2456] *Crystal::Parser#parse_logical_or:Crystal::ASTNode+ +70
[0xa2f207c] *Crystal::Parser#parse_cmp:Crystal::ASTNode+ +76
[0xa2f1c9c] *Crystal::Parser#parse_equality:Crystal::ASTNode+ +76
[0xa2f1af3] *Crystal::Parser#parse_and:Crystal::ASTNode+ +67
[0xa2f1953] *Crystal::Parser#parse_or:Crystal::ASTNode+ +67
[0xa2f1824] *Crystal::Parser#parse_range:Crystal::ASTNode+ +68
[0xa2f169c] *Crystal::Parser#parse_question_colon:Crystal::ASTNode+ +44
[0xa2f0281] *Crystal::Parser#parse_op_assign<Bool, Bool>:Crystal::ASTNode+ +193
[0xa2f01ac] *Crystal::Parser#parse_op_assign:Crystal::ASTNode+ +76
[0xa2f0133] *Crystal::Parser#parse_expression:Crystal::ASTNode+ +67
[0xa2ef6d0] *Crystal::Parser#parse_multi_assign:Crystal::ASTNode+ +96
[0xa2ef008] *Crystal::Parser#parse_expressions_internal:Crystal::ASTNode+ +248
[0xa2eeeeb] *Crystal::Parser#parse_expressions:Crystal::ASTNode+ +75
[0xa34007c] *Crystal::Parser#parse_module_def:Crystal::ModuleDef +300
[0xa2f85fd] *Crystal::Parser#parse_atomic_without_location:Crystal::ASTNode+ +18413
[0xa2f3d73] *Crystal::Parser#parse_atomic:Crystal::ASTNode+ +67
[0xa2f3d03] *Crystal::Parser#parse_atomic_with_method:Crystal::ASTNode+ +67
[0xa2f3837] *Crystal::Parser#parse_prefix:Crystal::ASTNode+ +215
[0xa2f3556] *Crystal::Parser#parse_pow:Crystal::ASTNode+ +70
[0xa2f325c] *Crystal::Parser#parse_mul_or_div:Crystal::ASTNode+ +76
[0xa2f2bdf] *Crystal::Parser#parse_add_or_sub:Crystal::ASTNode+ +95
[0xa2f2936] *Crystal::Parser#parse_shift:Crystal::ASTNode+ +70
[0xa2f26e6] *Crystal::Parser#parse_logical_and:Crystal::ASTNode+ +70
[0xa2f2456] *Crystal::Parser#parse_logical_or:Crystal::ASTNode+ +70
[0xa2f207c] *Crystal::Parser#parse_cmp:Crystal::ASTNode+ +76
[0xa2f1c9c] *Crystal::Parser#parse_equality:Crystal::ASTNode+ +76
[0xa2f1af3] *Crystal::Parser#parse_and:Crystal::ASTNode+ +67
[0xa2f1953] *Crystal::Parser#parse_or:Crystal::ASTNode+ +67
[0xa2f1824] *Crystal::Parser#parse_range:Crystal::ASTNode+ +68
[0xa2f169c] *Crystal::Parser#parse_question_colon:Crystal::ASTNode+ +44
[0xa2f0281] *Crystal::Parser#parse_op_assign<Bool, Bool>:Crystal::ASTNode+ +193
[0xa2f01ac] *Crystal::Parser#parse_op_assign:Crystal::ASTNode+ +76
[0xa2f0133] *Crystal::Parser#parse_expression:Crystal::ASTNode+ +67
[0xa2ef6d0] *Crystal::Parser#parse_multi_assign:Crystal::ASTNode+ +96
[0xa2eef60] *Crystal::Parser#parse_expressions_internal:Crystal::ASTNode+ +80
[0xa2eeeeb] *Crystal::Parser#parse_expressions:Crystal::ASTNode+ +75
[0xa2da4fc] *Crystal::Parser#parse:Crystal::ASTNode+ +60
[0xa49d2d0] *Crystal::TopLevelVisitor +944
[0x97e2dd8] *Crystal::ASTNode+ +2200
[0xa498cc9] *Crystal::TopLevelVisitor#visit<Crystal::Expressions>:Bool +153
[0x97e29c4] *Crystal::ASTNode+ +1156
[0xa49d34b] *Crystal::TopLevelVisitor +1067
[0x97e2dd8] *Crystal::ASTNode+ +2200
[0xa498cc9] *Crystal::TopLevelVisitor#visit<Crystal::Expressions>:Bool +153
[0x97e29c4] *Crystal::ASTNode+ +1156
[0xa49d34b] *Crystal::TopLevelVisitor +1067
[0x97e2dd8] *Crystal::ASTNode+ +2200
[0xa498cc9] *Crystal::TopLevelVisitor#visit<Crystal::Expressions>:Bool +153
[0x97e29c4] *Crystal::ASTNode+ +1156
[0x9290f3b] *Crystal::Program#top_level_semantic<Crystal::ASTNode+>:Tuple(Crystal::ASTNode+, Crystal::TypeDeclarationProcessor) +267
[0x9290591] *Crystal::Program#semantic<Crystal::ASTNode+, Bool>:Crystal::ASTNode+ +97
[0xa45c195] *Crystal::Compiler#compile<Crystal::Compiler::Source, String>:Crystal::Compiler::Result +405
[0x8966fef] *run<String, Nil, Bool, Crystal::Debug>:(Crystal::SpecRunOutput | LLVM::GenericValue) +735
[0x8966d02] *run<String>:(Crystal::SpecRunOutput | LLVM::GenericValue) +82
[0x89903cb] ~procProc(Nil) +43
[0x8965ed8] *it<String, String, Int32, Int32, &Proc(Nil)>:(Array(Spec::Result) | Nil) +600
[0x898ef76] ~procProc(Nil) +1142
[0x9237344] *Spec::RootContext::describe<String, String, Int32, &Proc(Nil)>:Spec::Context+ +484
[0x89667df] *describe<String, String, Int32, &Proc(Nil)>:Spec::Context+ +63
[0x891314d] ???
[0x8938d0d] main +77
[0xf6e8e637] __libc_start_main +247
[0x890ffab] ???

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Aug 8, 2017

Are we hitting the memory limit of Travis 32-bit VM? Maybe we should build std/compiler_spec.cr and std/std_spec.cr one after the other.

@RX14
Copy link
Contributor

RX14 commented Aug 8, 2017

@ysbaddaden I don't think that specific segfault is because of memory, but I may be proven wrong. Typically memory issues appear when fork() fails.

@straight-shoota
Copy link
Member

straight-shoota commented Aug 8, 2017

Maybe initialize a few rebuilds to determine if the error is persistent?

@RX14
Copy link
Contributor

RX14 commented Aug 8, 2017

@straight-shoota It's not, this error has been around for months.

@Sija
Copy link
Contributor Author

Sija commented Aug 9, 2017

@ysbaddaden @RX14 Coming back to the topic of this PR, shall we get this merged or shall I open another one, changing name conventions as noted in #4808 (comment)?

@RX14
Copy link
Contributor

RX14 commented Aug 9, 2017

Well we need each_child anwyay, why not combine all these name changes into 1 PR (could be this one)? You should look at the instance methods too, see if any of those should be renamed.

@Sija
Copy link
Contributor Author

Sija commented Aug 10, 2017

@RX14 done.

RX14
RX14 approved these changes Aug 10, 2017
@bew
Copy link
Contributor

bew commented Aug 10, 2017

You should look at the instance methods too, see if any of those should be renamed.

@Sija I think you missed that. Especially Dir#each should be renamed to Dir#each_entry.

Note: do we want to have #each_child #entries & #children instance methods as well?

@Sija
Copy link
Contributor Author

Sija commented Aug 10, 2017

@Sija I think you missed that. Especially Dir#each should be renamed to Dir#each_entry.

Thanks, you're right, I've changed it.

Note: do we want to have #each_child #entries & #children instance methods as well?

I'd say yes. WDYT @ysbaddaden @RX14 ?

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Good job!

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Aug 10, 2017

Usually we'll want to return an array for easyness (e.g. to pass it along to a template), sometimes we'll want to yield a folder and don't need an array (avoids HEAP allocation), and we may need the full directory listing, including . and .. once in a while (either returning an array or iterating).

@Sija
Copy link
Contributor Author

Sija commented Aug 10, 2017

@ysbaddaden said & done, ready for review :)

RX14
RX14 approved these changes Aug 10, 2017
src/dir.cr Outdated
excluded = {".", ".."}
entry = nil
loop do
break unless excluded.includes?(entry = @dir.read)
Copy link
Contributor

@RX14 RX14 Aug 10, 2017

Choose a reason for hiding this comment

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

Why not use early return here?

excluded = {".", ".."}
loop do
  entry = @dir.read
  return entry unless excluded.includes? entry
end

Copy link
Contributor Author

@Sija Sija Aug 10, 2017

Choose a reason for hiding this comment

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

fixed.

Copy link
Contributor

@bew bew Aug 10, 2017

Choose a reason for hiding this comment

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

Can I suggest another one?

excluded = {".", ".."}
while entry = @dir.read
  return entry unless excluded.includes? entry
end
stop

Nevermind you don't have to include that one, just wanted to write it somewhere..

Edit: oh in fact you implemented it that way too, perfect then 😆

@Sija Sija force-pushed the dir-each-child branch 2 times, most recently from 5a9f80b to 606f283 Compare Aug 10, 2017
RX14
RX14 approved these changes Aug 10, 2017
@Sija
Copy link
Contributor Author

Sija commented Aug 10, 2017

Anything more to do here, or is it GTG?

@RX14
Copy link
Contributor

RX14 commented Aug 10, 2017

@Sija, @ysbaddaden hasn't reviewed the latest changes.

@Sija
Copy link
Contributor Author

Sija commented Aug 12, 2017

@ysbaddaden ping

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Aug 13, 2017

Thank you!

@ysbaddaden ysbaddaden merged commit 8de1d8e into crystal-lang:master Aug 13, 2017
2 checks passed
@Sija
Copy link
Contributor Author

Sija commented Aug 13, 2017

still awesome experience, so yw, thx!

@ysbaddaden ysbaddaden added this to the Next milestone Aug 16, 2017
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.

None yet

5 participants