On 9/2/16, 5:10 PM, "Greg Dove" <greg.d...@gmail.com> wrote:
>Yes, that was one of the major changes. I moved it alongside the other >implementation inside ClassDirectiveProcessor and was able to get more >consistent results. >I initially commented it out so I could get everything working using the >implements IEventDispatcher approach - which is the more general approach >that would work for all cases- in javascript, then added it back for both >targets as a 2nd output variation in a later commit, and it went to >ClassDirectorProcessor for swf. > >I was seeing 2 issues from the ASCompilationUnit implementation: >1. It was not always applying where it should have been (so in cases where >it could have been 'extends EventDispatcher' it would sometimes fall >through to the implements IEventDispatcher approach in >ClassDIrectiveProcessor anyway). I suspect this was threading-related, but >I am not so familiar with threads. > >2. Less of an issue, it could also apply an EventDispatcher base class in >static-only bindable cases, which is unnecessary, this I expect could have >been more easily fixed. I tried checking with hasModifier in the original >code, but I think that might not be available yet, iirc that caused an >error. > >So I moved everything to the ClassDirectiveProcessor, which had the other >implementation of bindable support in any case. Sorry, I thought I had >been >clear about that. Well, you probably did explain it, but it probably didn't stick in my head until we hit this last issue and I went looking for the code where I had a vague memory of hacking a base class. I still don't know the compiler code so well that it is clear in my head what work should be done where. It looks like the original attempt to deal with [Bindable] for SWF was done in ClassDirectiveProcessor, but when I wanted to fix a bug in FalconJX I decided to fix it by hacking the AST since that drives everything in FalconJX. The right answer may have been for me to move the code from ClassDirectiveProcessor into ASCompilationUnit and use the existing tests for needsEventDispatcher, then maybe we wouldn't have had the two issues above. Anyway, I think we have recorded the possibility that AST hacking might be a solution if we hit other problems. I'm ok with leaving the code as is until we hit another problem but feel free to keep digging if you want. Thanks, -Alex
