409087 - Dehydra needs correct representation of nested classes

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Benjamin Smedberg]

Attachment: Testcase

Dehydra currently represents nested classes without information about the parent type, which leads to confusion when multiple inner classes have the same name.

The simple solution is I believe to call process_class with c.name == 'Outer::Inner' and not just 'Inner'.

The same thing will need to happen with namespaces, though I'm less concerned about that right now because I'm not experiencing conflicts.

Comment 1

[Benjamin Smedberg]

Seems like namespaces are fixed, so the only thing remaining is nested classes.

Comment 2

[David Mandelin [:dmandelin]]

Fix checked into hg.

Comment 3

[Benjamin Smedberg]

Now the class.name is correct, but the bases aren't the same:

GCHeap.h:82: ERROR: Class 'MMgc::GCHeap' has unknown base 'GCAllocObject'.

c.bases[0].name should be 'MMgc::GCAllocObject'

Comment 4

[Benjamin Smedberg]

This seems to fix it... I copied your change above. ok to commit?

diff --git a/dehydra_js.cc b/dehydra_js.cc
--- a/dehydra_js.cc
+++ b/dehydra_js.cc
@@ -755,7 +755,7 @@ bool JSVisitor::visitTypeSpecifier(TypeS
   for(FakeList<BaseClassSpec > *bs = c->bases; !bs->isEmpty();
       bs = bs->butFirst())
   {
-    JSString *str = JS_NewStringCopyZ(cx, bs->first()->type->name);
+    JSString *str = JS_NewStringCopyZ(cx, bs->first()->type->typedefVar->fullyQualifiedName0().c_str());
     xassert(JS_DefineElement(cx, basesArray, baseCount++, STRING_TO_JSVAL(str),
                              NULL, NULL, JSPROP_ENUMERATE));
   }

Comment 5

[David Mandelin [:dmandelin]]

(In reply to comment #4)
> This seems to fix it... I copied your change above. ok to commit?

Yes, that will work. I just grep'd for other occurences of type->name and this seems to be the only other one.

Comment 6

[David Mandelin [:dmandelin]]

bsmedberg reported that the gc analysis was having problems with the order in which dehydra calls inner and outer classes. The problem was that dehydra was calling process_class in the order of their first occurrence in the file, thus the inner class first. Analyses would generally want process_class called in the order in which definitions are completed (for the same reason as C++ compilers). 

Below is a patch that accomplishes this. It's a very simple change, but I didn't commit yet because I'm concerned some dehydra scripts might have a problem with this new version. In particular, process() is called for inline-defined member functions *before* process_class() is called for the class that contains them. Will this be a problem for anyone?

diff -r e56df4072e90 dehydra_js.cc
--- a/dehydra_js.cc	Thu Jan 03 10:15:18 2008 -0800
+++ b/dehydra_js.cc	Thu Jan 03 10:57:29 2008 -0800
@@ -704,11 +704,16 @@ void JSVisitor::addDeclarator(Declarator
 
 bool JSVisitor::visitTypeSpecifier(TypeSpecifier *obj) {
   TS_classSpec *c = obj->ifTS_classSpec();
-  if(!c || !(c->keyword == TI_STRUCT || c->keyword == TI_CLASS)) return true;
+  if (c) killImplicitMembers(c);
+  return true;
+}
+
+void JSVisitor::postvisitTypeSpecifier(TypeSpecifier *obj) {
+  TS_classSpec *c = obj->ifTS_classSpec();
+  if(!c || !(c->keyword == TI_STRUCT || c->keyword == TI_CLASS)) return;
   
-  killImplicitMembers(c);
   jsval process_class = getCallback("process_class");
-  if (process_class == JSVAL_VOID) return true;
+  if (process_class == JSVAL_VOID) return;
 
   this->loc = obj->loc;
   // create and append object to a rooted array(rooting it)
@@ -764,7 +769,6 @@ bool JSVisitor::visitTypeSpecifier(TypeS
                               1, argv, &rval));
   // trim the top of the array since we no longer care for them
   xassert(JS_SetArrayLength(cx, typeArrayArray, length));
-  return true;
 }
 
 // author: dmandelin
diff -r e56df4072e90 dehydra_js.h
--- a/dehydra_js.h	Thu Jan 03 10:15:18 2008 -0800
+++ b/dehydra_js.h	Thu Jan 03 10:57:29 2008 -0800
@@ -22,6 +22,8 @@ public:
   JSVisitor(DehydraCmd &cmd);
   ~JSVisitor();
   
+  virtual bool visitTypeSpecifier(TypeSpecifier *obj);
+  virtual void postvisitTypeSpecifier(TypeSpecifier *obj);
   virtual bool visitE_funCall(E_funCall *e);
   virtual bool visitE_variable(E_variable *e);
   virtual bool visitS_decl(S_decl *s);
@@ -36,7 +38,6 @@ public:
   virtual void postvisitExpression(Expression *e);
   virtual void postvisitE_fieldAcc(E_fieldAcc *e);
   virtual bool visitFunction(Function *obj);
-  virtual bool visitTypeSpecifier(TypeSpecifier *obj);
   virtual bool visitDeclarator(Declarator *d);
   virtual bool visitE_intLit(E_intLit *e);
   virtual bool visitE_deref(E_deref *e); // author: dmandelin

Comment 7

[Benjamin Smedberg]

Yeah, that's likely to be a problem. Is there a way to get this order:

process_class(Outer::Inner)
process(Outer::Inner::Inlinefunc)
process_class(Outer)
process(Outer::InlineFunc)

or

process_class(Outer::Inner)
process_class(Outer)
process(Outer::Inner::Inlinefunc)
process(Outer::InlineFunc)

Comment 8

[David Mandelin [:dmandelin]]

Attachment: patch for class order with methods after classes

Here's a patch that will call back js in order: (a) process_class in order that definitions are completed, (b) process() for a method after process_class for its owner. I haven't tested it extensively.

Comment 9

[David Mandelin [:dmandelin]]

(In reply to comment #8)
Patch committed to hg.