Daniel Russel wrote: >> Although everything compiles, MSVC spits out a bunch of warnings about >> the MKSUnit stuff > I looked this morning and, as far as I could tell, it was warning about > using a standard feature of C++ that I was intentionally using. MSVC has > ways to turning off specific warnings, so I may go and turn the warnings > off for those calls.
Good; that was my understanding too. It's unfortunate that MSVC sucks.
> The patch is a bit hazy on what NDEBUG should do (reflecting my > haziness). In general, NDEBUG should do or not do whatever we want it to > do (and turning it on shouldn't make code slower). Other than that, I > wouldn't worry too much about user expectations, especially since we > don't allow the users to control it directly.
The only thing we can say for sure is that when NDEBUG is defined, assert() is a noop. IMHO, IMP_assert() acts like assert(), so it should do the same thing - that's how I put it in the patch I committed. For the other runtime checks, it seems reasonable for it to be like CHEAP or NONE, as you say.
> From what I understand, the tests were exposing bugs in IMP. The > patches probably should have fixed the bugs and in case we should fix > the bugs in IMP rather than just disable the tests. Or am I confused.
You are probably confused because it did both. ;) The patch did fix the bug in IMP (use of invalidated iterators). It also disabled the particle deletion part of the tests, because IMP was touching freed particle pointers in that case. Arguably that's also a bug in IMP, but it was my understanding that we can't "fix" that without putting in particle reference counting, which I didn't have the time to do.
Ben