This repository has been archived by the owner on Jan 22, 2019. It is now read-only.
Initial code stub #38
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
(Created as follows: "doxygen -g")
These are based on settings use on December's work, as well as ones that were on Hugh Sorby's pull request.
This is to show that the test actually does test something.
This reverts commit bd4a32c.
All lowercase, "_" separates words. See cellml/libcellml#30
Was in Model.h in previous code, lost since rollback.
Separated cmake file for library from that for tests, both separate from top level cmake file.
As suggested by Hugh Sorby (@hsorby). This has a few benefits, for examp.e, it helps validate that the set of headers is correct, since the unit tests can only see the public headers. Previously, headers that were not intended to be part of the public API might also be in the `src` directory. (Obviously at this stage we only have one header file in the set of public headers, but the plural is used since this will change soon.)
Based on feedback. This improves the include approach as follows: 1) Public include directory is isolated (the api directory) 2) `#includes` will start with `libcellml/` to avoid name clashes and make modularity more self-documenting.
Thanks to Hugh for spotting this.
There was some debate about whether the word "usually" should be there. While I don't personally agree that it should be deleted, it is too small an issue to warrant further discussion.
As suggested by @hsorby.
Conflicts: doxy.config
Test target only generated if `enable_testing()` is in top-level file.
Also for Build dir, and only where appropriate.
…n when CMake version 3.2 is available.
…ult set by CMake.
…nsistency, README.md changes.
Closed
|
I think this sufficiently addresses #37 to be considered complete. I've managed to build it locally and it seems to work as expected (given the caveat about waiting on cmake 3.2 to get the C++11 checking/enabling properly implemented) and also passes on the ABI build server for all the platforms. I think @hsorby has managed to fix up all the issues that were holding up #28, so I believe this pull request to be suitable for merging in. |
|
@hsorby managed to fix up the few remaining issues. I fixed the bulk of them, (with lots of help from him). |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Initial code stub to satisfy issue #37.