Cleanup and change constraint names#168
Cleanup and change constraint names#168joaopapereira wants to merge 3 commits intocgreen-devs:masterfrom
Conversation
|
I believe the Shippable error is not related to my commit |
|
True. We have some problems with the Shippable configuration, and I've been meaning to remove it. |
|
Did you had some time to review this PR? |
matthargett
left a comment
matthargett
left a comment
There was a problem hiding this comment.
If Constraint is made a private header, how will users define custom constraints out-of-tree from CGreen? This is pretty common in the 3 companies I deployed cgreen at that did work in *BSD/Linux.
|
I think @matthargett 's comment need to be considered. We don't want to limit users possibility to extend Cgreens functionality. I did not consider this when I wrote the issue because I had not seen that used anywhere. Although you would still find the include file and use that, the "internal" part signals that it is not intended for external use, so if that scenario is to be supported, we should keep it where it is. As a sidenote I think it would be very valuable to have a section in the manual about extending Cgreen with custom constraints. That also signals that this is an intended API. (Think there are also some "API-checking type tests" somewhere which should include this.) Otherwise I think the name change is a worthwhile change, and also probably necessary if this should be API. So I suggest to keep the name change of If If you would implement special constraints, you might also need to include |
|
I think @thoni56 and @matthargett are right moving constraint.h into internal is hiding the fact that we can create new constraints. Also noticed that my patch had an inconsistency with names. In the code we use |
joaopapereira
force-pushed
the
cleanup-change-constraint-names
branch
from
7deda01 to
f3ea804
Compare
|
Any update on the review of this PR? |
joaopapereira
force-pushed
the
cleanup-change-constraint-names
branch
from
f3ea804 to
895d214
Compare
joaopapereira
force-pushed
the
cleanup-change-constraint-names
branch
from
895d214 to
6491297
Compare
Rename the file to `cgreen/constraints.h` so that it is clear this is the file that should included if we need to use constraints when doing tests. If the objective is to create user defined constraints the file that should be included is `cgreen/constraint.h`.
The function declaration of `get_significant_figures` and `significant_figures_for_assert_double_are` was moved to `internal/contraints.h` as the implementation is already in the corresponding .c file
The `Constraint` struct name can easily conflict with other application that have some sort of constraint. To ensure that name collision does not happen the struct was renamed from `Constraint` -> `CgreenConstraint`
joaopapereira
force-pushed
the
cleanup-change-constraint-names
branch
from
6491297 to
49e08d3
Compare
thoni56
force-pushed
the
master
branch
4 times, most recently
from
3d2b639 to
35ca914
Compare
4 participants
In this pull request I am doing the following changes:
constraint.htointernal/constraint.h(Fixes Cleanup include files wrt. constraints #52)internal/constraint.htointernal/constraints.h(Fixes Cleanup include files wrt. constraints #52)Constraintstruct toCGreenConstraintThe first 2 are part of the issue #52 the last one was something that we observed while using CGreen. The struct Constraint can already be defined in the users application which will cause an error while compiling the test.
To solve this issue the approach was to prefix the structure name with
CGreen