I just noted this discussion. I support an idea of checking thread's suspend enabled/disabled state automatically. It seems to be useful and convenient approach to me. To be honest I don't see huge difference between two names SuspendDisabledChecker & EnableDisableAutoAssert. Though the last one sounds better.
Thanks Evgueni On 11/28/06, Gregory Shimansky <[EMAIL PROTECTED]> wrote:
Geir Magnusson Jr. wrote: > > > Gregory Shimansky wrote: >> Geir Magnusson Jr. wrote: >>> >>> >>> Gregory Shimansky wrote: >>>> Geir Magnusson Jr. wrote: >>>>> >>>>> >>>>> Gregory Shimansky wrote: >>> >>>>>> >>>>>> 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 :) >>> >>> No - you're missing my point completely. it isn't about the commit >>> comments, but rather the source. Because of the unorthodox name of >>> the class and lack of any comment *in the code* it's unclear to a >>> reader what's going on. >>> >>> It was certainly unclear to me. >>> >>> The class is a cute trick, and I can see how it makes life easier for >>> maintenance, but we also have to consider that this code is being >>> written for collective ownership and maintenance, so the more >>> explicit and clear we can be, the better off the project is. >>> >>> Always write code assuming that you win the lottery and retire >>> tomorrow, so make it clear enough that there are few questions. >> >> If you grep the source you'll find that Suspend.*Checker classes are >> used a lot. Do you suggest to put a comment before every use? It is >> just the first time you've seen this call, so it was not clear to you. >> Simply finding the class definition would've answered all your >> questions. It is a normal thing to do when you see that some function >> is called, and don't understand what it does. > > Why use names at all then? Why not "A", "B", "C" for your > classnames...? (and "a", "b" "c" for your function names...) > > Then if someone points out that it made the code not obvious to the > reader, we can just tell them to use grep and look it up.... > > :D I didn't think that the name is so unclear, and the main problem was that it is unclear why define a local variable which is not used. >> >> I understand the need to document the code and put comments into it. I >> also think that vmcore/include/suspend_checker.h could have some >> doxyden docs. But putting identical comments like "check the that >> suspend is enabled/disabled when entering and leaving this function" >> all around the code, seems to be too excessive to me. >> > > How about renaming it? "EnableDisableAutoAssert" or something? Ok I'll put it on my todo list. -- Gregory
