Hi Janne,
thanks for the review. On 2012-10-14 23:35, Janne Blomqvist wrote:
- 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.
Ok to both. The bug is a real problem, and I don't think the patch is at all dangerous, but it's good to have a second opinion.
- 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)
Of course the BBT is equivalent to a map (or hash-table) with different syntax, but I agree that it would be nice to enhance the code to do away with brute-force searching.
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.
Yes, of course, we don't want to end up in a situation where gfortran maintainers suddenly need to know C, Fortran and all of C++, so I want to be careful about this patch. Besides that concern, I'll also wait until I get more input concerning the issue that Jakub raised.
Cheers, - Tobi
