On Mon, 2009-04-20 at 18:03 -0700, Jesse Jones wrote: > > On Apr 20, 2009, at 4:54 PM, Sebastien Pouliot wrote: > > > > > Hello Jesse, > > > > Another great idea - but same comment, as previous email, for > > "SideEffect" versus "Impure"* > > > > * feedback appreciated from everyone on the list :) > > > > On Mon, 2009-04-20 at 15:24 -0700, Jesse Jones wrote: > >> This is a companion to my earlier rule. It's designed to catch code > >> like this: > >> > >> // This is really bad. > >> Debug.Assert(text[++i] == ' '); > >> > >> and > >> > >> // This will probably work, but it's definitely not intended and may > >> // hide bugs. > >> Debug.Assert(flag = flag2); > >> > >> The idea here is that the rule should check for calls to a > >> conditionally compiled method and complain if an expression used to > >> compute an argument stores a value into a local, argument, or field. > >> > >> I think the only germane C# expressions are pre/post-increment, > >> decrement, and assign. The new operator is the only other possibility > >> I see, but I think we'd see way too many false positives if we > >> included it. > > > > I think that locals are more likely to bring false positives (than > > new), > > since some will be defined (e.g. in #if DEBUG blocks) specifically for > > that purpose. > > It's going to take some testing, I think, to see if checking locals is > a good idea.
Yes. I don't want to exclude it - but I just wanted to say that it's something that will need to be looked at before committing the rule. Mono's source code is big, and varied, enough for this kind of check. > But I think this case will (almost always) work because > the local will normally be given its value outside the conditional > method call. Likely > > It may be worth to include both (locals and new) to see how much false > > positives you get. Then either drop them (if really bad) or drop the > > confidence to low. > > I'm not sure how many false positives new would give. But the other > problem is that almost all news are going to be innocuous. The only > ones that aren't are the ones which call a constructor which modifies > an object other than the one it is constructing. This seems like a > rare enough case that the potential for false benefits outweighs the > benefits (even at low confidence). Good point - and it should be easy to confirm while checking for the locals. Thanks, Sebastien --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Gendarme" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/gendarme?hl=en -~----------~----~----~----~------~----~------~--~---
