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 <https://github.com/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
