Keith, This is responding to all your remaining comments, except unittesting.py, which I will leave Luis or Evan to discuss.
On 01/04/10 15:12, Keith Mitchell wrote: > Hi Evan (and the rest of the Error Service project team), > > I took a good look at errsvc.py, and a briefer look at the C files > (except for the test_*.c files). My comments are below. Looks pretty good! > > - Keith > > errsvc.py: > > General nit: There are some extra spaces around parens in this file > (e.g. at lines 38, 57, 110, etc.) will fix. > 40-42: Can we define this list as a constant somewhere? ok - will do. > 49-55: Is there any reason to not use direct attribute access? I think it's fairly standard to provide setter and getter methods in cases like this. Also, it means the C and Python user code is similar because there's a function to call in both cases. > 57: Why the use of return codes in Python code? This function raises an exception if an invalid "type" param was passed in (ie a programming error), but returns 0 if the value doesn't match the type (a possible runtime scenario). That seems about right to me. And again, it means that the C and Python user code will be more similar. > > 65: Why this syntax? If this list is expected to grow later, can we > define it as a constant of the class? ok - will do. > 88: Performance is likely not an issue here, but in general, string > concatenation is much more efficient if you create a list of strings, > and then use "\n".join(<list>) or something similar. Right - this function is only used for testing, so it's not a performance issue. > 110-118: Could this be incorporated into the ErrorInfo.__init__? Or, is > it a valid scenario to create an ErrorInfo independently from the list? I think it's neater to have a function to call here. And again, it makes the C implementation easier by having a function to call. > 133: Since we control the adding of errors to the list, would it be > easier to simply store separate lists for each type? Or have the global > __errors be a dictionary, where the key is error_type and the value is a > list of errors of that type? It seems that conglomerating lists together > when the full list of all errors is needed would be simpler than picking > apart the list to get errors of a certain type. > > 143: Same question. Since we control adding items to __errors via the > create_error_info method, it seems like managing multiple lists (or > dictionaries, as appropriate) would be relatively simple. It could be done that way, but I don't see anything wrong with the current implementation. Bear in mind that in practice there will rarely be more than one or two errors in existence at any time. So, I don't think re-writing for maximum efficiency is necessary. > 157: The second check (errs > 0) is redundant - "if errs:" only returns > True if a list is non-empty. good catch - will fix > 158: Nit: "for err in errs:" yes - will fix > 159: Nit: "print err" is sufficient. (And, using str(err) instead of > err.__str__() is generally more common) ok - will fix > 178: Nit: integer -> string > > 163-185: Can you use the Python built-in isinstance instead? good suggestion - will change. > unittesting.py: > > General: The error cases should be tested too (e.g., calling > set_error_data with an invalid type, or with a value that mismatches the > type, or instantiating an ErrorInfo with an invalid type). > > 31-32: Nit: platform.processor() is simple enough to make a call to, to > determine sparc or i386. > 41: Nit: Instead of sys.exit(1), re-raise the exception (or raise a new > one). Calling sys.exit(1) prevents, for example, writing a meta-module > that runs a series of unittests (if the import of this module failed, > and a reasonable exception is raised, the managing module/script can > catch it and move on to the next set of tests - if sys.exit(1) is > called, the rest of the script wouldn't get run). > > 49: Nit: Call it "self.error_types", since it's a list with more than > one type. > > 52: Why store this value? It's easily retrievable with a call to len() > > 57, 67, 72: Use "for error_type in self.error_types" instead of creating > the arbitrary index variable c. It's rare to need to know the index of > an item in a list in Python (and when you do, use of the enumerate() > function is the best way to track it when looping - line 116 is a good > example: "for idx, item in enumerate(self.error_types):") > > 66, 96, 115: This should be in a tearDown method. > > 140: Should this be a liberrsvc constant? > > errsvc.c: > > General question: What's the advantage/purpose of the use of threading > in this code? > > General note: There are a few comments that are just "Call Python" - it > would help if the comment were expanded to indicate what Python code is > called (especially since the Python method called can get obscured by > the fact that it takes quite a few lines of C code to get to the point > of being able to call said method) > > 61: What's the purpose of hardcoding the path here? Seems like this > would cause maintenance problems and reduce flexibility. > > 110: "UNKNOWN ERROR" - should this be #defined? > > 566-571: I don't quite understand the comment. It seems like calling the > clear_error_list function should take care of clearing out the error > list. Where are the extra references coming from that need to be > decremented here, instead of by whichever portion of the code grabbed > hold of the extra reference in the first place? > > 683: As mentioned above, Python code should raise errors, and not return > error codes. Reserve return values for useful data, and check for > exceptions if attempting to handle an error condition. > > 1030-1078: This bit of code looks very similar to blocks of code in > other functions. Is it possible to combine somehow for the purpose of > simplifying, or are they different enough that it's not possible to do > such a thing in C? > > liberrsvc.h:60: Nit: _LIBERREVC_H -> _LIBERRSVC_H thanks - will fix > > liberrsvc_defs.h: > > 38-40, 41-42: Nit: Make these into single block comments so they're more > readable. ok - will fix > prototype_com: > > General: The osol_install directory seems to be getting a little > cluttered, perhaps we need a 'utils' directory for things like the error > service, the future logging service, and so on? see previous email from Evan. Thanks again for reviewing. - Dermot > > Evan Layton wrote: >> The CUD Error Service project team is requesting a code review of >> the project gate. >> >> The code implementation is complete and unit testing has been done. >> The official testing is in progress but is not yet complete. We will >> be providing updated incremental webrevs as code review comments and >> bug fixes come in. >> >> The webrev is available at: >> http://cr.opensolaris.org/~evanl/CUD_errsvc >> >> With the holiday coming up it appears that most people will be out >> of town for a while so we'll be giving extra time for folks to look >> at this. We'd like to have all comments back by January 15, 2010. >> >> Thanks! >> >> -evan >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
