Geir Magnusson Jr. wrote:
Gregory Shimansky wrote:
Geir Magnusson Jr. wrote:
Gregory Shimansky wrote:
On Sunday 26 November 2006 03:30 Geir Magnusson Jr. wrote:
1) Why add the SuspendDisabledChecker if not using it?
2) exactly where did you add the assertion? :)
It is a hidden assertion class :)
Oh, come on. An assertion as a side effect?
No, assertion is one and the only purpose of this class. Isn't word
Checker in class name is not descriptive enough.
No, clearly not :)
Look at the file vm/vmcore/include/suspend_checker.h. There are 2
classes SuspendEnabledChecker and SuspendDisabledChecker. When
declared, such variable in constructor checks for suspend status, in
destructor it checks that the status is the same. It is often
convenient to write just this one line to make sure that all returns
from the function have the same suspend status because local
variable destructor is executed on every function exit.
In release, when assert is a noop, these constructors/destructors
are optimized into noop as well.
I saw that this function uses raw ManagedObject pointers. This is
dangerous in case when suspend is enabled (equal to GC being
enabled) as the object may be moved at any time. So I decided to add
this assertion. If it fails some time it will signal that this
function is called in a wrong unsafe mode.
Ok - but don't you think that the assert() would be clearer and just
as efficient?
In this function maybe, but in other places where it is used, the fact
that all return sites in the function are covered by this one line is
very convenient. Also if function changes, someone may forget to put
assertion before some return statement.
I'll admit, it is darn convenient.
In VM code SuspendChecker usage is clear enough, it is quite widely
used, although there is a lot of code which wasn't converted since
checker classes were introduced. New code uses them, old code uses
assertions.
It's not clear to a casual reader. I don't know if a rename is in
order, or the addition of a comment when used, but clearly to claim that
you added an "assert" and then use a class called "Checker" is far from
transparent.
geir
I'll try to make more descriptive commit comments :)
--
Gregory
