These are chat archives for Snaipe/Criterion

3rd
Jun 2016
Dominik
@kaidowei
Jun 03 2016 08:33
suggestion:
int strcmp_safe(const char *s1, const char *s2)
{
    if (s1 == s2)
        return 0;
    if (!s1)
        return -1;
    if (!s2)
        return 1;
    return strcmp(s1, s2);
}
problem is, that you can provide actual and expected parameters to the assertions, that will pass the assertion, although one string is NULL and the code might crash in production
for example
char *actual = function_that_gives_null_instead_of_aaa();
char *expected = "bbb";
cr_assert_str_lt(actual, expected);
Dominik
@kaidowei
Jun 03 2016 09:48
for the other point: why is it unfortunate, if strings are passed via nanomsg?
Dominik
@kaidowei
Jun 03 2016 10:08
if I remember correctly, criterion passes file contents too, right?
Franklin Mathieu
@Snaipe
Jun 03 2016 11:08
Unfortunate in the sense that the message for cr_assert_str_eq("hallo", "hello") will be "The expression (as strings) ("hallo") == ("hello") is false: actual='hallo' expected='hello'.".
but it's not really an issue tbh
it's just that the information may get repeated
leave the format like that for the moment
This will get addressed at another time

problem is, that you can provide actual and expected parameters to the assertions, that will pass the assertion, although one string is NULL and the code might crash in production

If we define the semantics of string comparison to accept NULL, this isn't much of a problem

if a function is expected to return NULL, it the user's responsibility to call cr_assert_not_null(str) or cr_assert_str_neq(str, NULL)
Franklin Mathieu
@Snaipe
Jun 03 2016 11:15
You're right that this might otherwise come out as a surprise, but this is the most consistent solution if we are going to check for NULL
So this will have to be, of course, documented
another possibility would be to return a special value and make the assertion fail regardless if one of the parameters is NULL
but I think that ordering the strings such as NULL < "" < "a" < ... makes sense.
Franklin Mathieu
@Snaipe
Jun 03 2016 11:22
although... now that I think about it, I don't think that this would make any comparison with NULL really viable
what would cr_assert_str_lt(str, NULL) really achieve other than being a overengineered cr_abort
Dominik
@kaidowei
Jun 03 2016 11:23
well, the NULL could be hidden in a function or variable
Franklin Mathieu
@Snaipe
Jun 03 2016 11:24
yeah, that's mostly the problem
so, okay, the best way would be to support (s1 == s2) for NULL checks, and otherwise abort the test if either s1 or s2 is NULL
Dominik
@kaidowei
Jun 03 2016 11:26
can we provide another message, if one parameter is null
Franklin Mathieu
@Snaipe
Jun 03 2016 11:27
Sure
you'd just call a cr_fail from within strcmp_safe
the only issue I see with this is that this is going to hard fail the test on cr_expect_str_{lt,gt,leq,geq}
but I don't think we can sanely handle that with the current implementation
Dominik
@kaidowei
Jun 03 2016 11:30
int strcmp_safe(const char *s1, const char *s2)
{
    if (!s1 || !s2)
        cr_fail(...);
    return strcmp(s1, s2);
}
Franklin Mathieu
@Snaipe
Jun 03 2016 11:30
that would be it, with the if (s1 == s2) return 0; at the begining
Dominik
@kaidowei
Jun 03 2016 11:31
can we give parameters to strcmp_safe and cr_fail to mimic being outside of the function?
Franklin Mathieu
@Snaipe
Jun 03 2016 11:31
otherwise this would also hard fail for cr_expect_eq/neq
Yes, you could, but the implementation would need to change a bit
you'd need to pass the line number + the path of the source file as parameters
and then you'd need to make a specialized cr_assert_impl_
maybe change its definition to take in a Line and File parameter
Dominik
@kaidowei
Jun 03 2016 11:33
hmm. I'm not happy with that.
Franklin Mathieu
@Snaipe
Jun 03 2016 11:33
and make a convenience macro that passes __LINE__ and __FILE__ to that
This is a bit hacky-ish for the current structure to be honest
Dominik
@kaidowei
Jun 03 2016 11:34
why is it a problem to have two asserts/expects instead of one?
Franklin Mathieu
@Snaipe
Jun 03 2016 11:35
It messes up the reported statistics
cr_assert_str_eq should be reported as one assertion, not multiple ones
because, in its nature, this represents "one data check"
plus, you would need to do two check ideally (one for each parameters)
so this would make one assertion, in fact, three
it isn't much of a problem when using the default reporter, but for things like the xml or json reporter, that writes entries for each assertion, this will output surprising results
Dominik
@kaidowei
Jun 03 2016 11:39
and if we change the check?
Franklin Mathieu
@Snaipe
Jun 03 2016 11:40
Oh, actually, you have a point
I don't know why I didn't think of that
Dominik
@kaidowei
Jun 03 2016 11:40
CR_STDN (Actual) && (Expected) && strcmp((Actual), (Expected)) Op 0
Franklin Mathieu
@Snaipe
Jun 03 2016 11:40
just pass a (Actual) != NULL && (Expected) != NULL && strcmp(...)
yeah, that
CR_STDN would have to be in front of strcmp, but that would work
the only issue I see is that this won't get reported with <s1> is NULL or <s2> is NULL
but it's not a problem with the new format you introduced
Dominik
@kaidowei
Jun 03 2016 11:43
but the assertion fails and you'll see the actual='NULL'
Franklin Mathieu
@Snaipe
Jun 03 2016 11:43
yeah
Dominik
@kaidowei
Jun 03 2016 11:43
right :D
Dominik
@kaidowei
Jun 03 2016 11:57
# define cr_assert_str_op_empty_(Fail, Op, Msg, Value, ...) \
    CR_EXPAND(cr_assert_impl(                               \
            Fail,                                           \
            ((Value) != NULL) && (Value)[0] Op '\0',        \
            dummy,                                          \
            ((Value) != NULL) ? Msg: CRITERION_ASSERT_MSG_IS_NULL, \
            (CR_STR(Value), Value),                         \
            __VA_ARGS__                                     \
    ))
providing a better message. Do like that or should I revert the Msg line?
might be a problem, because msg needs two strings and CRITERION_ASSERT_MSG_IS_NULL needs one
Franklin Mathieu
@Snaipe
Jun 03 2016 11:58
yeah, this wouldn't work for that reason
Dominik
@kaidowei
Jun 03 2016 11:59
well, I just tested it and (for me) it works :D
we can further hack: the second message gets another %s
and we provide "" if actual is NULL
but yeah, might get too complicated
Franklin Mathieu
@Snaipe
Jun 03 2016 12:00
Actually it wouldn't really matter in this one instance
because passing more parameters isn't an error
(and it itsn't UB either)
so, you're good to go in this case
Dominik
@kaidowei
Jun 03 2016 12:01
yeah.
I'll do some more checks and update soon...
Dominik
@kaidowei
Jun 03 2016 12:11
@Snaipe another problem... after make clean, I can't run tests anymore.
$ ctest
Test project /home/dweidemann/Criterion
      Start  1: signal.c
Could not find executable signal.c.bin
Looked in the following places:
signal.c.bin
signal.c.bin
Release/signal.c.bin
Release/signal.c.bin
Debug/signal.c.bin
Debug/signal.c.bin
MinSizeRel/signal.c.bin
MinSizeRel/signal.c.bin
RelWithDebInfo/signal.c.bin
RelWithDebInfo/signal.c.bin
Deployment/signal.c.bin
Deployment/signal.c.bin
Development/signal.c.bin
Development/signal.c.bin
Unable to find executable: signal.c.bin
 1/38 Test  #1: signal.c .........................***Not Run   0.00 sec
Franklin Mathieu
@Snaipe
Jun 03 2016 12:11
make criterion_tests
Dominik
@kaidowei
Jun 03 2016 12:12
:/ ok
can you link that to make test?
Franklin Mathieu
@Snaipe
Jun 03 2016 12:12
make doesn't build the tests by default
it's planned
but surprisingly hard as cmake doesn't provide this by default
it's a shame, really
Dominik
@kaidowei
Jun 03 2016 12:14
okay, thanks...
one more thing: for translation, I just edit the *.po file?
Franklin Mathieu
@Snaipe
Jun 03 2016 12:15
Yes