Dear Nick,

Thank you for reviewing this.
My comments:

— I did not want to introduce another dependency for m_io. This write operation is very localized in time, so there is no need to reserve
a unit in the way of m_io.
- My json schema is not perfect: the “info nodes: time, calls, and %, are logically also children of the section objects, but conceptually different (they are “leaf info” instead of tree branches. So with a view to a future scripting capability of the json file, I put the “_” in front to clearly mark them as different. This is obviously not fool-proof. 
- I agree that we do not need two different optional arguments.

Let me know if you agree, and I will just merge the interface simplification from your changes.

  Best regards,

  Alberto

> On 19 Feb 2018, at 09:21, Nick Papior <email address hidden> wrote:
> 
> I have made a merge request that tries to solve some of the things I have remarked below.
> 
> Note they are comments/remarks. I however think the logic is easier by not having two optional arguments.
> 
> Diff comments:
> 
>> 
>> === modified file 'Src/timer_tree.f90'
>> --- Src/timer_tree.f90	2016-01-25 16:00:16 +0000
>> +++ Src/timer_tree.f90	2018-02-15 15:48:27 +0000
>> @@ -266,31 +269,71 @@
>>     enddo
>>     p%data%totTime = globaltime + 1.0e-6_dp
>> 
>> +    json_output = .true.
>> +    call get_unit_number(js_lun)
>> +    open(unit=js_lun,file="time.json",form="formatted", &
>> +         action="write",position="rewind")
>> +    
>>     write(*,"(/,a20,T30,a6,a12,a8)") "Section","Calls","Walltime","%"
>> -    call walk_tree(p,0)
>> 
>> +    write(js_lun,*) '{'
>> +    call walk_tree(p,0,json=json_output,js_lun=js_lun)
>> +    write(js_lun,*) '}'
>> +    if (json_output) close(js_lun)
>> +    
>>   end subroutine timer_report_global
>> 
>>   !------------------------------------------------
>> -  recursive subroutine walk_tree(p,level,maxlevel)
>> +  recursive subroutine walk_tree(p,level,maxlevel,json,js_lun)
>>     type(section_t), intent(in),target  :: p
>>     integer, intent(in)          :: level
>>     integer, intent(in), optional:: maxlevel
>> +    logical, intent(in), optional:: json
>> +    integer, intent(in), optional:: js_lun
>> 
>>     integer :: i
>> -    character(len=40) fmtstr
>> +    character(len=40) fmtstr, fmt_json, fmt_json_head
>> +    logical :: json_output
>> 
>> +    json_output = .false.
>> +    if (present(json)) then
>> +       json_output = json
>> +       if (.not. present(js_lun)) then
>> +          call die("Need a js_lun for json timer output")
>> +       endif
>> +    endif
>>     if (present(maxlevel)) then
>>        if (level > maxlevel) RETURN
>>     endif
>>     pd => p%data
>>     write(fmtstr,"(a,i0,a1,a)") "(", level+1, "x", ",a20,T30,i6,f12.3,f8.2)"
>> +    
>>     write(*,fmtstr) pd%name, pd%nCalls,  &
>>                     pd%totTime, 100*pd%totTime/globaltime
>> +    if (json_output) then
>> +       write(fmt_json,"(a,i0,a1,a)") "(",2*level+1,"x",",a,i0,a,f12.3,a,f8.2)"
>> +       write(fmt_json_head,"(a,i0,a)") "(", 2*level+1, "x,a)"
>> +       write(js_lun,fmt=fmt_json,advance="no") &
>> +           '"' // trim(pd%name) // '": { "_calls": ', pd%nCalls,   &
>> +                                      ', "_time": ', pd%totTime,   &
>> +                                      ', "_%": ', 100*pd%totTime/globaltime
>> +    endif
> 
> Why are they prefixed with "_", wouldn't "calls", "time" and "%" make more sense?
> 
>>     if (p%nchildren /= 0) then
>> +       if (json_output) write(js_lun,"(a)") ","
>>        do i=1,p%nchildren
>> -          call walk_tree(p%child(i),level+1,maxlevel)
>> +          call walk_tree(p%child(i),level+1,maxlevel,json,js_lun)
>> +          if (json_output) then
>> +             if (i < p%nchildren) then
>> +                write(js_lun,fmt="(a)") ','
>> +             else
>> +                write(js_lun,*)  ! just new line
>> +             endif
>> +          endif
>>        enddo
>> +       ! End the parent section with an indented '}'
>> +       if (json_output) write(js_lun,fmt=fmt_json_head,advance="no") "}"
>> +    else
>> +       if (json_output) write(js_lun,fmt="(a)",advance="no") "}"
>>     endif
>> 
>>   end subroutine walk_tree
>> @@ -353,4 +396,18 @@
>>     endif
>>   end subroutine current_time
>> 
>> +subroutine get_unit_number(lun)
> 
> Is there a reason not to use m_io instead of get_unit_number?
> 
>> +integer, intent(out) :: lun
>> +
>> +logical :: used
>> +integer :: iostat
>> +
>> +do lun= 10, 99
>> +   inquire(unit=lun, opened=used, iostat=iostat)
>> +   if (iostat .ne. 0) used = .true.
>> +   if (.not. used) return
>> +enddo
>> +call die("Cannot get unit for timer output")
>> +end subroutine get_unit_number
>> +
>> end module m_timer_tree
> 
> 
> -- 
> https://code.launchpad.net/~albertog/siesta/4.0-json-time/+merge/337797
> You are the owner of lp:~albertog/siesta/4.0-json-time.