On Thu, Aug 4, 2016 at 5:11 PM, Barret Rhoden <[email protected]> wrote:
> On 2016-08-04 at 14:54 Dan Cross <[email protected]> wrote: > > > > On lines 13-15 of user/parlib/include/parlib/dtls.h, __GNUC__ is > sadly > > > > defined by several compilers (GCC, clang, and the Intel C compiler). > Is > > > it > > > > really that we care about GCC, or is it just that we care about being > > > > GCC-compatible? (I assume the latter.) > > > > > > You'd have to ask Kevin. This code originally came from Linux Parlib. > > > > > > I guess a larger question is if we'd ever try to compile this with a > > compiler for which this wouldn't work. Perhaps we should remove the > > '#error' and ifdef's, since they don't actually test for exactly GCC. > > I imagine we'll run into a lot of things similar to that throughout the > code base. > > > > On line 20 of user/utest/dtls.c I think it would be slightly more > > > > informative to say, "Expected NULL ..." instead of "expected 0" to > > > > accentuate that it's looking at a pointer, not an int. > > > > > > > I don't see a change on github; did you push it up? > > Nope. I also don't plan to. Sorry if 'Noted' wasn't clear in that > regard. Meh; that's too bad. Tests tend to set up conditions, perform some action, and assert that the outcome of the action was as expected. Asserting that the proper conditions are actually set before performing the action makes the test stronger when it breaks for whatever reason: did it break because the conditions were not set properly, or because the thing being probed didn't work? Similarly with the NULL thing; saying what's really happening unambiguously is useful (in text, a 0 can could just be some random number. NULL is obviously a pointer). > > On lines 77-85, it's unclear to me what the expected behavior of setting > > > > from a dtor is; could you add an assert to probe that? > > > > > > It is expected to not loop infinitely or to lose storage. > > > > > > Calling pthread_setspecific() from a thread-specific data > > > destructor routine may result either in lost storage (after at > > > least PTHREAD_DESTRUCTOR_ITERATIONS attempts at destruction) or > > > in an infinite loop. > > > > > > Kevin designed it to not fail in that manner. We can notice the > > > infinite loop, but not the storage loss. > > > > Ah. It is worth adding a comment to the test case to explain that. > > Not sure this is worth doing. The test is called "set from dtor". We > test if we can do that. Not to belabor the point but the test didn't actually assert anything so it was not clear what it was accomplishing. Obviously it does something, but does not confirm whether that thing was done or otherwise had any effect; in the absence of a statement of some kind of purpose, the reader would naturally wonder why one would bother.... The comment you added makes it much clearer. There's lots of other ways the test could > fail. Should we enumerate those? I don't know, but that's irrelevant: a test with no asserts that does something but doesn't probe that that thing worked or otherwise explain why it doesn't assert something is not self-explanatory, thus a comment helps explain purpose. This test originally failed due to a > page fault, and only because I gave it a weird value (15) for the > set_val and was doing a wild memory access. > > Anyway, I added a comment on that test. > Thanks! - Dan C. -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
