Hello Michal,ok I will have a look on your changes in the next days ;) Thank you for your efforts ;) Thierry.
Date: Wed, 10 Dec 2014 13:33:38 +0100 Subject: Re: PDT : Questions about type inference evaluators From: [email protected] To: [email protected] CC: [email protected] Hi Thierry,After this conversation I had bug report from user about CA and arrays issue. I fixed this bug, but I also did refactoring we were talking about here. Probably reorganization can go further, but I think for one step it is enough. Please look at these changes in your free time (no rush) and if you will find something suspicious let me know. https://bugs.eclipse.org/bugs/show_bug.cgi?id=454710 Thanks,Michal On Sun, Dec 7, 2014 at 10:05 PM, Michał Niewrzał <[email protected]> wrote: This mailing list is for sharing questions and ideas so don't hesitate to use it :) Michal On Sun, Dec 7, 2014 at 9:52 PM, thierry blind <[email protected]> wrote: Hi Michal, > Yup, from time to time I'm working with inference evaluators, but I'm not expert and I don't know full > history:) I'm still gathering knowledge and most of the time I'm trying to resolve simple bug. Your > questions are completely correct and reasonable:) I'm also gathering knowledge ;) What I really miss in PDT is that code is poorly commented/documented (but on the other side, I'm not a big fan about over-commenting code). >> - why aren't splitted values (using String#split("\\|")) tested if they are >> empty or not ? > I'm also not sure, but I can imagine that someone assumed that it wont break > code. ... or nobody didn't think about those case ;) > General truth is that there are lots of things to refactor/review, but we need time for it:) I will try in near > future look at these classes you mention because CA is one of most important part for users and making > things more consistent is always good idea:) Thank you for looking into it, even in a far future :) I wrote you this mail also to keep a trace about my questions & ideas. And you're right, it's a long-term work to have a deep look into so many code and to try to refactor it to gain in quality. Let's just do it (little) step by step ;) Thierry. Date: Sun, 7 Dec 2014 20:20:26 +0100 Subject: Re: PDT : Questions about type inference evaluators From: [email protected] To: [email protected] CC: [email protected] Hi Thierry, Yup, from time to time I'm working with inference evaluators, but I'm not expert and I don't know full history:) I'm still gathering knowledge and most of the time I'm trying to resolve simple bug. Your questions are completely correct and reasonable:) So my (stupid) questions are : - why do some evaluators look after brackets (MethodReturnTypeEvaluator, PHPDocMethodReturnTypeEvaluator, PHPDocClassVariableEvaluator) and others not (ClassVariableDeclarationEvaluator, VariableReferenceEvaluator) ? I'm not sure. It is possible that when someone were fixing one bug related to arrays type declaration then were not checking every possible related evaluator assuming bug is only in this specific case. It needs to be reviewed. - why aren't splitted values (using String#split("\\|")) tested if they are empty or not ? I'm also not sure, but I can imagine that someone assumed that it wont break code. - why are PHPDocClassVariableEvaluator#getArrayType(String type, IType currentNamespace, int offset) and PHPDocMethodReturnTypeEvaluator#getArrayType(String type, IType currentNamespace, int offset) different ? I don't know:) Look below. - so getEvaluatedType(String typeName, IType currentNamespace) and getArrayType(String type, IType currentNamespace, int offset) from classes PHPDocClassVariableEvaluator and PHPDocMethodReturnTypeEvaluator can probably be merged (into a class utility?). It probably needs to be checked carefully but at first sight you are right and these method should be merges and extracted to be common for both classes. General truth is that there are lots of things to refactor/review, but we need time for it:) I will try in near future look at these classes you mention because CA is one of most important part for users and making things more consistent is always good idea:) Thanks,Michal On Sun, Dec 7, 2014 at 2:36 AM, thierry blind <[email protected]> wrote: Hello Michal how are you ? I've seen that you recently worked on some patch to correct problems with type inference evaluators (Bug 453737 - Tag @var doesn't work well with array types). I ran FindBugs some time ago and found some bugs related to the usage of String#split(). Maybe they are still some pending bugs concerning type inference evaluators, but I do not feel comfortable enough about these parts of code, so maybe you could help ;) What I find strange is the way that some evaluators (from packages org.eclipse.php.internal.core.typeinference.evaluators.*) are splitting types using String#split("\\|") and handling brackets (for array type declarations). So my (stupid) questions are : - why do some evaluators look after brackets (MethodReturnTypeEvaluator, PHPDocMethodReturnTypeEvaluator, PHPDocClassVariableEvaluator) and others not (ClassVariableDeclarationEvaluator, VariableReferenceEvaluator) ? - why aren't splitted values (using String#split("\\|")) tested if they are empty or not ? - why are PHPDocClassVariableEvaluator#getArrayType(String type, IType currentNamespace, int offset) and PHPDocMethodReturnTypeEvaluator#getArrayType(String type, IType currentNamespace, int offset) different ? - so getEvaluatedType(String typeName, IType currentNamespace) and getArrayType(String type, IType currentNamespace, int offset) from classes PHPDocClassVariableEvaluator and PHPDocMethodReturnTypeEvaluator can probably be merged (into a class utility?). I made some (untested) changes and provide them "as is" as attachments, so you can see what I mean. I also didn't put them on gerrit because I don't plan to make a patch ;) Thierry.
_______________________________________________ pdt-dev mailing list [email protected] To change your delivery options, retrieve your password, or unsubscribe from this list, visit https://dev.eclipse.org/mailman/listinfo/pdt-dev
