410685 - ordering of process_class in the presence of templates

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Benjamin Smedberg]

Attachment: dehydra-test-early-template.ii testcase

In dehydra: given two classes:

template<typename T>
class B : public T
{
};

class A
{
};

void Func()
{
  B<A> b;
}

process_class is called for B<class A> before it is called for A. This sucks, because the .bases refers to classes which aren't available yet. I think that we should ensure that process_class is called for template dependencies before the templated class itself.

Comment 1

[David Mandelin [:dmandelin]]

Eek. I think the way to fix it is to make Dehydra keep track of each template instantiation and what definitions it depends on, and call process_class after the last dependence has been processed. I can't quite convince myself this works in all cases--I'll need to do some extra testing.

Comment 2

[David Mandelin [:dmandelin]]

Attachment: patch (goes on top of other order patch)

This patch should fix the problem. Note: it goes on top of the other patch for inner/outer class order. 

This version only makes sure that base classes are visited before derived classes. It doesn't enforce order for other instantiations. Let me know if you need that or any other constraints added.

Comment 3

[Benjamin Smedberg]

Yes, I'm going to need this for members that are of class type or array-of-class-type as well... not pointer or reference type or arrays of those. Or to put it another way, if a forward-declared type is not allowed in that context, then I need that type to be processed first.

template<class T>
class A
{
  T mT; // T can't be forward-declared, so I need process_class(T) first
};

class B
{
};

void Foo()
{
  A<B> a;
}

You can check this against 'nsBaseHashtableET<class nsDepCharHashKey, nsAutoPtr<struct nsINIParser::INIValue> >' which needs to come before 'nsBaseHashtable<class nsDepCharHashKey, nsAutoPtr<struct nsINIParser::INIValue>, struct nsINIParser::INIValue *, class CAllocator>'

Comment 4

[Benjamin Smedberg]

oh, and the patch does work as far as it goes... I'm impressed that it took so little time to implement!

Comment 5

[David Mandelin [:dmandelin]]

Attachment: Patch for member ordering

Yet another patch for the stack. This makes sure that classes referred to directly or in an array in a member are processed first. I didn't test it on all of FF but I did test on a small file and on nsINIParser.ii with find-stack-comptrs.js.

Note: Before this patch, typedef members were included in the member list passed to process_class. I took them out because (a) I'm guessing they shouldn't be there, and (b) they make find-stack-comptrs.js fail because it tries to analyze the typedef'd classes, although they may not be defined yet, and don't have to be according to C++. If for some reason they should be in there, let me know how you want them to show up.

Another thing: in testing find-stack-comptrs.js I saw a bunch of warnings generated for missing union types. I could add a property to indicate they are unions, or have a process_union called for them, or something like that, if you want.

Comment 6

[David Mandelin [:dmandelin]]

I pushed the last 2 changes into hg, which should take care of all the issues raised so far.

Comment 7

[Benjamin Smedberg]

I'll call this FIXED and file followups if I find further issues. Thanks!