On Sat, Oct 13, 2012 at 4:26 PM, Tobias Schlüter <tobias.schlue...@physik.uni-muenchen.de> wrote: > > Hi, > > first a question also to non-gfortraners: if I want to use std::map, where > do I "#include <map>"? In system.h? > > Now to the patch-specific part: in this PR, module files are produced with > random changes because the order in which symbols are written can depend on > the memory layout. This patch fixes this by recording which symbols need to > be written and then processing them in order. The patch doesn't make the > more involved effort of putting all symbols into the module in an easily > predicted order, instead it only makes sure that the order remains fixed > across the compiler invocations. The reason why the former is difficult is > that during the process of writing a symbol, it can turn out that other > symbols will have to be written as well (say, because they appear in array > specifications). Since the module-writing code determines which symbols to > output while actually writing the file, recording all the symbols that need > to be written before writing to the file would mean a lot of surgery. > > I'm putting forward two patches. One uses a C++ map to very concisely build > up and handle the ordered list of symbols. This has three problems: > 1) gfortran maintainers may not want C++isms (even though in this case > it's very localized, and in my opinion very transparent), and > 2) it can't be backported to old release branches which are still > compiled as C. Joost expressed interested in a backport. > 3) I don't know where to #include <map> (see above) > Therefore I also propose a patch where I added the necessary ~50 lines of > boilerplate code and added the necessary traversal function to use > gfortran's GFC_BBT to maintain the ordered tree of symbols. > > Both patches pass the testsuite and Joost confirms that they fix the problem > with CP2K. I also verified with a few examples that they both produce > identical .mod files as they should. > > Is the C++ patch, modified to do the #include correctly, ok for the trunk? > If not, the C-only patch? Can I put the C-only patch on the release > branches? And which?
Hi, I'm pleasantly surprised that you managed to fix this PR with so little code! - Personally, I'd prefer the C++ version; The C++ standard library is widely used and documented and using it in favour of rolling our own is IMHO a good idea. - I'd be vary wrt backporting, in my experience the module.c code is somewhat fragile and easily causes regressions. In any case, AFAICS PR 51727 is not a regression. - I think one could go a step further and get rid of the BBT stuff in pointer_info, replacing it with two file-level maps std::map<void*, pointer_info*> pmap; // Or could be std::unordered_map if available std::map<int, pointer_info*> imap; So when writing a module, use pmap similar to how pointer_info BBT is used now, and then use imap to get a consistent order per your patch. When reading, lookup/create mostly via imap, creating a pmap entry also when creating a new imap entry; this avoids having to do a brute-force search when looking up via pointer when reading (see find_pointer2()). (This 3rd point is mostly an idea for further work, and is not meant as a requirement for accepting the patch) Ok for trunk, although wait for a few days in case there is a storm of protest on the C vs. C++ issue from other gfortran maintainers. -- Janne Blomqvist
