37 fortran interface null terminated strings #41
Conversation
There was a problem hiding this comment.
Changes were tested with my fortran test case which times a main program plus three subroutines, and it works absolutely fine (see attached image above). It just 'magically works' without me having to make any changes to how the profiler is called which is nice.
The code itself makes sense and formatting is kept consistent, I only left a couple of comments which are more queries from me rather than huge issues - it clearly works OK as is.
…_name` argument.
There was a problem hiding this comment.
Looks good on the whole, just a couple of points to address. Returning to @mo-mglover.
cmake/Doxygen.cmake
Outdated
| @@ -21,6 +21,8 @@ function(enable_doxygen) | |||
| set(DOXYGEN_JAVADOC_BLOCK NO) | |||
| set(DOXYGEN_FULL_PATH_NAMES NO) | |||
| set(DOXYGEN_STRIP_CODE_COMMENTS NO) | |||
| set(DOXYGEN_FILE_PATTERNS *.c *.cpp *.h *.f90 *.F90 ) | |||
There was a problem hiding this comment.
Because you're now defining the specific file extensions Doxygen no longer picks up the .md file used for the front page, adding *.md to this list should fix this though it will add the directory to the list of files shown in the Doxygen pages even if here isn't anything there.
src/f/profiler_mod.F90
Outdated
| !----------------------------------------------------------------------------- | ||
| contains | ||
|
|
||
| !> @brief Start a profiled region, taking care to add a C null character to |
There was a problem hiding this comment.
The @brief here should be for the user, describing that the subroutine is adding a null character doesn't seem right for the API documentation. A normal comment inside the subroutine would probably be better i think.
There was a problem hiding this comment.
I'll move it into a @note. I think it's important that it mentions the null character on the tin: it tells the Fortran programmer that they need not add one explicitly in higher-level code.
There was a problem hiding this comment.
Sounds good to me.
andrewcoughtrie
added
under review
andrewcoughtrie
added
ready to be merged
and removed
under review
andrewcoughtrie
deleted the
37-fortran-interface-null-terminated-strings
branch
Wraps the
profiler_startinterface with a subroutine that concatenatesc_null_charonto the end of the region name. It also changes thenameargument toregion_nameso as to not clash with a piece of Fortran syntax. (It shouldn't cause an issue, but just to be on the safe side.)There is also a change to the Doxygen configuration. These changes are a tad tangential, but I thought I'd include them while I was making changes to the comment blocks in
profiler_mod. I noticed that later versions of Doxygen (installed on site) don't recognise.F90as being Fortran files; hence they didn't appear in the output documentation. The Doxygen configuration changes represent a patch for that.Reason I was trying later Doxygen versions: there doesn't seem to be away of getting comment blocks for Fortran interfaces to appear in the Doxygen output. Not even with
@fn. There's a wider issue open already (#38) covering Doxygen stuff as a whole. Getting that to work is beyond the scope of the present changes.