Serialise empty model to XML #49

codecurve · 2015-03-27T03:11:05Z

This is for #31

Schema has been used to generate XML serialisation/ deserialisation code, based on Cellml_1_2.xsd. Notes: - CodeSynthesis XSD used as follows: ./generate.sh This creates Cellml_1_2.cpp and .h. - This approach of starting with an essentially blank schema document is not my preferred approach, but I was outvoted. My personal preference would have been to work incrementally, starting with the existing schema for CellML 1.1 and making only the changes that pertain to the differences between CellML 1.1 and the draft for CellML 1.2. A demonstration of that was done as shown here: https://github.com/codecurve/libcellml/blob/04de33845306a6468d389fd8a1be25e7405221e7/src/cellml_1_2.xsd as well as the commits that followed it.

Test was previously intended to fail.

As per http://stackoverflow.com/a/6847817 Note: this had to be redone, since the CMake refactor, it somehow got lost, see 89c624b

agarny · 2015-03-29T21:02:14Z

A few remarks:

Shouldn't the .cpp/.h files generated by CodeSynthesis XSD be named cellml_1_2.cpp and cellml_1_2.h rather than Cellml_1_2.cpp and Cellml_1_2.h, respectively (i.e. all lower case)? Maybe this can be done by renaming Cellml_1_2.xsd to cellml_1_2.xsd?
In fact, why the reference to 1.2? Shouldn't we drop that reference altogether? Just wondering.
Comments: I can see some comments ending with . (e.g. https://github.com/codecurve/libcellml/commit/45aaaa9c4c54e880d8955a40afb676f9c2d41d97#diff-54c9d32c0cd4684d21e2a2a8ba949cb9R28) and others not (e.g. https://github.com/codecurve/libcellml/commit/45aaaa9c4c54e880d8955a40afb676f9c2d41d97#diff-54c9d32c0cd4684d21e2a2a8ba949cb9R29). Is that intentional? Might be good to be consistent?
Just wondering: is the code currently following the Google coding style (as I believe it has been agreed?)? Considering that some styles have been changed, I would think so. So, just want to make sure that it is the case.
Wrong indentation? https://github.com/codecurve/libcellml/commit/45aaaa9c4c54e880d8955a40afb676f9c2d41d97#diff-c2834ecd762e5cef4f316b2c5981f557R25.
Some using namespace are still being used. See https://github.com/codecurve/libcellml/commit/45aaaa9c4c54e880d8955a40afb676f9c2d41d97#diff-5421c51c47d2405f1f9d861d1ecb8b8fR26.
The auto keyword should only be used to avoid clutter (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#auto). I am not sure this is the case at https://github.com/codecurve/libcellml/commit/45aaaa9c4c54e880d8955a40afb676f9c2d41d97#diff-5421c51c47d2405f1f9d861d1ecb8b8fR33 for example?
Might want to create a CMake macro for things that are going to be common to both libCellML and its tests. A candidate for this would be https://github.com/codecurve/libcellml/commit/37f26017b1a5d822bc6528572f5d99232357a076.
BuildBot: it would seem that we should upgrade the version of CMake to 3.2 from 3.1.1 on the Ubuntu VM.
BuildBot: need to resolve issues of unresolved external symbols.

@agarny

As suggested by @agarny as part of review on #49.

@agarny

As suggested by @agarny on review comment for #49.

@agarny

As suggested by @agarny on review comment for #49.

@agarny

As suggested by @agarny on review of #49.

codecurve · 2015-03-29T22:05:18Z

Thanks @agarny
I've renamed to cellml_1_2.*, as you suggested (i.e. starting on lowercase) (see a357443).
I think we plan to support multiple versions of CellML, so it will be necessary to distinguish these, hence the _1_2 for v1.2 is necessary (IMHO).
Yes, I am attempting to follow the Google style guide.
I've changed the use of auto that you highlighted to explicitly stating the type, and also to using a const reference, which is more correct, thanks for suggesting that. (see 2154983)
Last week I asked @hsorby to upgrade the Ubuntu cmake to v3.2, I think he might do it today.
I've eliminated use of using namespace directives (see 4b12cc5)
I've made the comments consistently end with full stops (see c4431f0).
I'm waiting for @hsorby for access to the Windows Build slave to try and work out why it has unresolved symbols during linking, since it links fine on my Windows box.
The "wrong indentation" is required in order to use the raw string literals. For the tests I did when I previously made quite a bit of progress serialising CellML v1.2 models with components, units and variables, I used raw string literals for the expected XML strings, and it is far more readable (see https://github.com/codecurve/libcellml/blob/e7a0fc5fe60be981f82da481710c05d8a473babb/tests/XmlBindingsSerialisationTest.cpp#L14-L21 for example)

Also, copy semantics better than reference for std::string in this case, I think, since move constructor will be invoked.

codecurve · 2015-03-29T22:37:56Z

@agarny Good catch, thanks, I've addressed that now. (See cf391bb).
I guess it is a matter of taste. My personal guideline is to try and reduce clutter, so if I'm only resolving scope once, no using is needed, but more than once seems to reduce clutter, IMHO.

codecurve · 2015-03-29T22:41:57Z

I've managed to reproduce the failed Windows link on my Windows PC by setting the LIBCELLML_ENABLE_SHARED cmake flag. I'm trying @agarny's suggestion as shown here:
https://github.com/opencor/opencor/blob/master/src/plugins/miscellaneous/Compiler/src/compilerglobal.h#L25
and here
https://github.com/opencor/opencor/blob/master/src/plugins/miscellaneous/Compiler/src/compilerengine.h#L55

codecurve · 2015-03-29T23:26:39Z

Windows build now working, see 89b2f09.

This is in order to get Ubuntu build slave result. (This will be reverted once cmake update done on build slave).

agarny · 2015-03-29T23:58:25Z

I seem to remember reading somewhere that there is a need for CMake 3.2, so how come everything is building fine using CMake 3.1.1 on Ubuntu (through BuildBot)?

Otherwise, I can see that there is a warning on the Ubuntu build: http://autotest.bioeng.auckland.ac.nz/libcellml-buildbot/builders/Unit%20Tests%20Builder%20Ubuntu%2014.04%2064%20Bit/builds/86/steps/compile/logs/stdio. I don't know what has been agreed, if anything, regarding warnings, but personally I would consider them as errors (i.e. build libCellML using /WX or -Werror on Windows and Linux/OS X, respectively).

This eliminates the warning on Linux that a deprecated header was included.

codecurve · 2015-03-30T01:32:44Z

Ubuntu cmake 3.1.1 seems to do mostly the correct thing, but 3.2 is required on Windows and OS X.
I've eliminated the warning, see 799e632.
As for setting the flags so warnings are treated as errors, I'm neutral, but I do think that is a separate issue and pull request, so I'll add an issue for it.

codecurve · 2015-03-30T01:37:20Z

I've added #50 regarding the "compile warnings as errors" question.

DRY (Don't Repeat Yourself) principle: set flags in one place and reuse from other locations.

codecurve · 2015-03-30T03:51:11Z

"CMake macro for things that are going to be common" - done, see 54de041.
#50 - done, as discussed with @agarny we decided to tack it onto this one, rather than make a separate pull request.

jonc125 · 2015-03-30T08:44:53Z

On the using question, if a particular term is being used multiple times within a single function then it's reasonable to start the function with a using ns1::ns2::term for instance. The downside of doing so at a whole file level is that it's less clear what's in scope, and hence harder to read & reuse code. A using namespace declaration is even worse from this perspective, and even at a function level can lead to issues from scope pollution.

@agarny

Thanks @agarny for catching that. Note: Warnings are now treated as errors, and build fails.

Note that the in-memory representation of the model is unused. I think this is probably OK, since there is only one way of serialising an empty model anyway.

hsorby · 2015-03-30T21:52:05Z

I'm not a big fan of this approach with regards to the bulk setting of the compiler flags. I think this should be set on a per target basis.

The conditional tests for compiler options should be based on the compiler and not the platform.

This is no longer needed:

To work around a bug conditionally set the CXX_STANDARD property

if("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux")
set_target_properties(cellml PROPERTIES CXX_STANDARD 11 CXX_STANDARD_REQUIRED ON)
endif()

I don't understand why this header guard has been changed to this:

#ifndef SRC_API_LIBCELLML_VERSION_H_

I would also set the CMAKE_MODULE_PATH and then just include the common.cmake file.

The name of the macro is misleading it really should be set_compiler_flags, and as I said earlier this should be done on per target basis.

codecurve · 2015-03-30T22:14:36Z

The header guard is in the format required by the Google style guide as I understand it: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#The__define_Guard

hsorby · 2015-03-30T22:15:07Z

The document string in this line isn't correct.

set( CMAKE_CODEBLOCKS_EXECUTABLE "" CACHE INTERNAL "Internalise BUILD_SHARED_LIBS" FORCE )

codecurve · 2015-04-01T03:17:20Z

Done, see b603ca1

(In reply to cellml/libcellml#49 (comment))

agarny · 2015-04-01T03:43:28Z

Fine by me.

codecurve · 2015-04-02T00:01:17Z

20 hours have passed since all review items have been addressed (as far as I can tell). I think this can be merged, please.

…el-01

hsorby · 2015-04-02T00:11:58Z

the header file ${CMAKE_CURRENT_SOURCE_DIR}/cellml_1_2.h is listed as an API header file but this is not the case

hsorby · 2015-04-02T00:13:15Z

the ${CMAKE_INSTALL_PREFIX}/ is not required in the install(FILES command

@hsorby

As suggested by @hsorby

codecurve · 2015-04-02T00:23:50Z

Done, thanks. See 6896bff.

the header file ${CMAKE_CURRENT_SOURCE_DIR}/cellml_1_2.h is listed as an API header file but this is not the case

@hsorby

It gets prepended anyway, remaining string is a relative path. @hsorby pointed this out, thanks.

codecurve · 2015-04-02T00:32:28Z

Done, see f7bcb5b

the ${CMAKE_INSTALL_PREFIX}/ is not required in the install(FILES command

…el-01

hsorby · 2015-04-02T01:27:20Z

I have created another pull request onto @codecurve empty-model-01 branch with the changes to the CMake related files that make me happy.

A few other points:

I would think the generated files should be in a directory named 'generated' under the source tree
the xsd, generate.sh script and xsd-options.txt files placed in a 'utils' directory as a sibling of the src directory
the type-naming and function-naming options for the generation be set to 'knr' and not 'java'
- not so fussed on this it seems you gain a little with the method names but lose on class names (although I do think java sends the wrong message :) )

Changes to CMake related files

…include directories. It would appear gtest is not very conformant to CMake best practices.

Got a bit carried away with my deletions

@hsorby

As suggested by @hsorby.

… into empty-model-01 Conflicts resolved: src/CMakeLists.txt

codecurve · 2015-04-02T03:54:40Z

Done, see 49051a6 and 3677047

A few other points:

I would think the generated files should be in a directory named 'generated' under the source tree

the xsd, generate.sh script and xsd-options.txt files placed in a 'utils' directory as a sibling of the src directory

codecurve · 2015-04-02T03:57:52Z

I chatted with @hsorby, and we agreed that keeping XSD generator options for "type-naming" and "function-naming" set to java is the best option for generated code, knr is too different from our target style, and it is not worth the cost to customise generation to exactly match our target style.

the type-naming and function-naming options for the generation be set to 'knr' and not 'java'

@hsorby

Rather than by adding `generated` as include search path. That way, it is clearer in source code when generated code is being used. @hsorby suggested this.

codecurve · 2015-04-03T16:22:21Z

Buildbot fail was not due to any problem in code, but due to download problem.
Passed on Window and Linux slaves, and builds and tests pass on my mac.
Sadly, there is no way yet to trigger a retry of a given build.

nickerso · 2015-04-04T00:48:11Z

HI Randall, we can manually trigger a changeset to be redelivered - just ask. In this case though, the download problem for the mac machine is still there.

codecurve · 2015-04-04T07:52:23Z

I have tried to access the Mac build slave that is having trouble with the download, but either my password has been changed, or I forgot it. Anyway, I have a mirror branch in my repo (https://github.com/codecurve/libcellml/tree/empty-model-0-travis-on-02), which has Travis enabled, and as you can see, both Mac and Linux builds passing there. See https://travis-ci.org/codecurve/libcellml/builds/57134328

Randall Britten added 3 commits March 27, 2015 16:08

Fixed implementation code to pass test.

9c4f117

Test was previously intended to fail.

Enabled MSVC exceptions.

37f2601

As per http://stackoverflow.com/a/6847817 Note: this had to be redone, since the CMake refactor, it somehow got lost, see 89c624b

codecurve mentioned this pull request Mar 29, 2015

Use case 1.i: Create and serialise empty model to XML #31

Open

Randall Britten added 4 commits March 30, 2015 10:16

Renamed cellml_1_2.* to start with lowercase letter

a357443

As suggested by @agarny as part of review on #49.

Changed from using auto to explicitly stating type

2154983

As suggested by @agarny on review comment for #49.

Eliminated using namespace as per Google style.

4b12cc5

As suggested by @agarny on review comment for #49.

Made use of full stop at end of comments consistent.

c4431f0

As suggested by @agarny on review of #49.

Removed unneeded scope resolution.

cf391bb

Also, copy semantics better than reference for std::string in this case, I think, since move constructor will be invoked.

Using version.h's technique for exporting symbols

89b2f09

Faking Cmake required version.

00e7344

This is in order to get Ubuntu build slave result. (This will be reverted once cmake update done on build slave).

Used sstream instead of deprecated strstream.

799e632

This eliminates the warning on Linux that a deprecated header was included.

Refactored common cmake settings.

54de041

DRY (Don't Repeat Yourself) principle: set flags in one place and reuse from other locations.

Randall Britten added 2 commits March 31, 2015 09:34

Actually call the macro defined in cmake common.

2c6df65

Thanks @agarny for catching that. Note: Warnings are now treated as errors, and build fails.

Eliminated unused variable.

357be47

Note that the in-memory representation of the model is unused. I think this is probably OK, since there is only one way of serialising an empty model anyway.

codecurve mentioned this pull request Mar 30, 2015

Cmake config to treat compile warnings as errors #50

Open

Minor refactoring of Cmake: reduce duplication.

2461bad

Fixed indentation

0d16eae

hsorby added 2 commits April 2, 2015 13:08

Use a list to group compilers that use the same flags.

dfbff0f

Merge remote-tracking branch 'coworker/empty-model-01' into empty-mod…

d855b08

…el-01

Removed header that is private from api headers.

6896bff

As suggested by @hsorby

Removed redundant path prefix.

f7bcb5b

It gets prepended anyway, remaining string is a relative path. @hsorby pointed this out, thanks.

hsorby added 5 commits April 2, 2015 13:41

Make use of generator expressions to simplify the configuration.

b85989f

Merge remote-tracking branch 'coworker/empty-model-01' into empty-mod…

4d9a4a2

…el-01

Correct documentation string for CMAKE_CODEBLOCKS_EXECUTABLE

114bfce

Don't need to add gtest as a target_link_library

f334ce0

Change out-of-date comment in tests/CMakeLists.txt

5126e94

codecurve and others added 6 commits April 2, 2015 15:08

Merge pull request #10 from hsorby/empty-model-01

17e077b

Changes to CMake related files

Got a bit carried away with my deletions, reinstating required gtest …

b5b46d7

…include directories. It would appear gtest is not very conformant to CMake best practices.

Fix typo

89515bf

Merge pull request #11 from hsorby/empty-model-01

4451c98

Got a bit carried away with my deletions

Moved XSD generated code, script and options.

49051a6

As suggested by @hsorby.

Merge branch 'empty-model-01' of https://github.com/codecurve/libcellml…

3677047

… into empty-model-01 Conflicts resolved: src/CMakeLists.txt

Specified generated header path in include.

075fac3

Rather than by adding `generated` as include search path. That way, it is clearer in source code when generated code is being used. @hsorby suggested this.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialise empty model to XML #49

Serialise empty model to XML #49

codecurve commented Mar 27, 2015

agarny commented Mar 29, 2015

codecurve commented Mar 29, 2015

codecurve commented Mar 29, 2015

codecurve commented Mar 29, 2015

codecurve commented Mar 29, 2015

agarny commented Mar 29, 2015

codecurve commented Mar 30, 2015

codecurve commented Mar 30, 2015

codecurve commented Mar 30, 2015

jonc125 commented Mar 30, 2015

hsorby commented Mar 30, 2015

codecurve commented Mar 30, 2015 via email

hsorby commented Mar 30, 2015

codecurve commented Apr 1, 2015

agarny commented Apr 1, 2015

codecurve commented Apr 2, 2015

hsorby commented Apr 2, 2015

hsorby commented Apr 2, 2015

codecurve commented Apr 2, 2015

codecurve commented Apr 2, 2015

hsorby commented Apr 2, 2015

codecurve commented Apr 2, 2015

codecurve commented Apr 2, 2015

codecurve commented Apr 3, 2015

nickerso commented Apr 4, 2015

codecurve commented Apr 4, 2015

Serialise empty model to XML #49

Are you sure you want to change the base?

Serialise empty model to XML #49

Conversation

codecurve commented Mar 27, 2015

agarny commented Mar 29, 2015

codecurve commented Mar 29, 2015

codecurve commented Mar 29, 2015

codecurve commented Mar 29, 2015

codecurve commented Mar 29, 2015

agarny commented Mar 29, 2015

codecurve commented Mar 30, 2015

codecurve commented Mar 30, 2015

codecurve commented Mar 30, 2015

jonc125 commented Mar 30, 2015

hsorby commented Mar 30, 2015

To work around a bug conditionally set the CXX_STANDARD property

codecurve commented Mar 30, 2015 via email

hsorby commented Mar 30, 2015

codecurve commented Apr 1, 2015

agarny commented Apr 1, 2015

codecurve commented Apr 2, 2015

hsorby commented Apr 2, 2015

hsorby commented Apr 2, 2015

codecurve commented Apr 2, 2015

codecurve commented Apr 2, 2015

hsorby commented Apr 2, 2015

codecurve commented Apr 2, 2015

codecurve commented Apr 2, 2015

codecurve commented Apr 3, 2015

nickerso commented Apr 4, 2015

codecurve commented Apr 4, 2015