<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On 05 Sep 2010, at 05:04, Hans-Peter Diettrich wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>Jonas Maebe schrieb:<br><blockquote type="cite">I think it is quite systematic and logical that the ppu loading</blockquote><blockquote type="cite">should not depend on code generator internals.<br></blockquote><br>Right, that's how it *should* be designed.</div></blockquote><div><br></div><div>That's how it is in trunk.</div><br><blockquote type="cite"><div>But try to find out why the code generation is added, when variables like current_filepos or current_tokenpos are moved into TModule (current_module) :-(<br></div></blockquote><div><br></div><div>Nobody ever said that it would be easy to restructure the compiler. In fact, I will be the first to admit that it is incredibly hard, and since the compiler is 15 years old (although several parts have been rewritten over the years), there are bound to be quite a few less than ideal situations and things that simply grew over time. However, I very much dislike the way you are doing the development and find it very hard to see how we are ever going to be able to integrate any of those changes back into trunk.</div><div><br></div><div>Maybe I simply should stop being involved in this, since I'm obviously not able to productively deal with this project.</div><br><blockquote type="cite"><div><blockquote type="cite">You have to describe in your svn commit logs what you actually do.<br></blockquote><blockquote type="cite">"NG: fixed make all" is not a useful commit message. You changed 149<br></blockquote><blockquote type="cite">files in that commit, and did not describe what was done nor why:<br></blockquote><br>I only commited these changes so that they don't get lost.</div></blockquote><div><br></div><div>You should never commit unrelated changes in a *single* commit. Even if you don't want to create a separate branch for other changes, at the very least use separate commits (although unrelated changes will make merging more difficult in case later changes also depend on them).</div><br><blockquote type="cite"><div>Please review the preceding version, as mentioned in the Readme.<br></div></blockquote><div><br></div><div>The main problem is that it cannot be merged since it doesn't compile. As I mentioned earlier: the ideal situation are piecemeal changes that form a tested and self-contained whole that can be easily reviewed and merged. While a single change related to global variables can obviously have repercussions all over the compiler, that does not mean that you have to replace all global variables that at first sight should end up in tscanner with fields in one go, especially if you are unable to completely test those changes (make all, make fullcycle, running the testsuite).</div><div><br></div><blockquote type="cite"><div>The last change was to remove ppudump from the Makefile, and this proved that so far only ppudump is sensitive to changes in the compiler internals.<br></div></blockquote><div><br></div><div>It also proves that the internal compiler structure has been broken.</div><div><br></div><blockquote type="cite"><div><blockquote type="cite">* you<br></blockquote><blockquote type="cite">deleted the aasmsym unit and changed code that depended on it without<br></blockquote><blockquote type="cite">any remarks why you did that. The comment in the header of that unit<br></blockquote><blockquote type="cite">said: "Contains abstract assembler instructions for all processor types, including routines which depend on the symbol table. These<br></blockquote><blockquote type="cite">cannot be in aasmtai, because the symbol table units depend on that<br></blockquote><blockquote type="cite">one."<br></blockquote><br>I wanted to make code generation more abstract, so that it can be separated better from the remaining code.</div></blockquote><div><br></div><div>Removing an extra abstraction class and folding it into the base class is the opposite of making something more abstract.</div><br><blockquote type="cite"><div>The GlobVar unit is intended as a place for all (previously) global variables, that can be used without importing further dependencies.<br></div></blockquote><div><br></div><div>The aasmsym unit did not contain any global variables. And putting all global variables in a single unit may not be the right approach since the compiler consists of many components. They are obviously interwoven, but making them more interwoven by basically making all types of all global variables globally visible does not sound a like a good idea.</div><br><blockquote type="cite"><div><blockquote type="cite">You nevertheless did move that code to aasmtai and now there are<br></blockquote><blockquote type="cite">indeed (implementation-level) circular dependencies between aasmtai<br></blockquote><blockquote type="cite">and symsym. Adding extra circular dependencies to the compiler<br></blockquote><blockquote type="cite">without any argument as to why this is required is not good.<br></blockquote><br>I don't remember details.</div></blockquote><div><br></div><div>That's why you should write it in the svn log. Note that you can edit the fpc repository's svn log entries even after you have committed, using "svn pe -r <revnr> --revprop svn:log".</div><br><blockquote type="cite"><div>My experimental approach left these units unmodified, and still resulted in importing code generation (see above).<br></div></blockquote><div><br></div><div>That's because of other changes. Making the problem worse does not contribute to the solution.</div><br><blockquote type="cite"><div><blockquote type="cite">* that<br></blockquote><blockquote type="cite">commit also contains changes that have nothing to do with fixing<br></blockquote><blockquote type="cite">"make all", such as fixing a typo in a comment, removing type<br></blockquote><blockquote type="cite">redefinitions, commenting the assignment of tcgaddnode to caddnode in<br></blockquote><blockquote type="cite">the init code of ncgadd.pas (and similar assignments in ncgmat),<br></blockquote><blockquote type="cite">possibly an unrelated change in ppheap (which is wrongly indented),<br></blockquote><blockquote type="cite">removing an unused local type definition from ppu.pas...<br></blockquote><br>I tried to follow the coding style, to not comment anything in the code.<br></div></blockquote><div><br></div><div>Oh, please.</div><br><blockquote type="cite"><div>Please suggest an better and more systematic way of reducing the number of compiler hints, and supply a justification for redefinitions of system unit types.<br></div></blockquote><div><br></div><div>I'm not saying that the local type definitions should stay. I'm saying that removing them must not be committed together with completely unrelated changes.</div><br><blockquote type="cite"><div><blockquote type="cite">* I assume<br></blockquote><blockquote type="cite">that all the code you added in psystem.pas is with the long term view<br></blockquote><blockquote type="cite">of making the parser independent. However, that is a separate project<br></blockquote><blockquote type="cite">and not part of removing global variables from the compiler<br></blockquote><br>Both are related, since the "stateful" variables are related to current_module, and that's where the scanner and parser is involved, somehow.<br></div></blockquote><div><br></div><div>That does not answer my remark, but I don't know how to make my point clearer.</div><br><blockquote type="cite"><div><blockquote type="cite">While reviewing your previous commits, I noticed that you also<br></blockquote><blockquote type="cite">integrated your patch from<br></blockquote><blockquote type="cite"><a href="http://bugs.freepascal.org/view.php?id=16888">http://bugs.freepascal.org/view.php?id=16888</a> although that one has<br></blockquote><blockquote type="cite">nothing to do with removing the use of globals.<br></blockquote><blockquote type="cite">This way of working is not how we can keep the compiler development<br></blockquote><blockquote type="cite">manageable for multiple people to work on.<br></blockquote><br>I added some improvements and corrected typos, while waiting for comments on my work. It would be nice to know in *advance*, what should be avoided.</div></blockquote><div><br></div><div>* don't add cyclic dependencies: <a href="http://lists.freepascal.org/lists/fpc-devel/2010-July/021065.html">http://lists.freepascal.org/lists/fpc-devel/2010-July/021065.html</a></div><div>* commit unrelated changes separately (even to separate branches if possible), polish/finish one self-contained part at a time so it can be merged: <a href="http://lists.freepascal.org/lists/fpc-devel/2010-August/021558.html">http://lists.freepascal.org/lists/fpc-devel/2010-August/021558.html</a></div><div><br></div><div>I may indeed not have mentioned before that svn commit messages should explain the idea behind the changes and why some things are done in a certain way.</div><br><blockquote type="cite"><div>The lack of response to my repeated review requests is not what I consider as "multiple people working on the same project".<font class="Apple-style-span" color="#000000"><font class="Apple-style-span" color="#144FAE"><br></font></font></div></blockquote><div><br></div><div>There are more people that you alone, and many people have spent a lot of time on answering many questions from you. Reviewing a bunch of changes with little or no information about why which changes were done is</div><div>a) a lot of work</div><div>b) not very enticing</div><br><blockquote type="cite"><div>In the meantime I started several projects, not commited to the repository, one of which (almost only) moves global variables into TModule, without breaking ppudump and affecting unrelated units. If you think that this is a better base for cooperation, I can try to add it as another branch, with no detailed history.<br></div></blockquote></div><br><div>I don't know.</div><div><br></div><div><br></div><div>Jonas</div></body></html>