Changeset c570806 for src/AST

Timestamp:

Aug 15, 2019, 11:45:56 AM (6 years ago)

Author:

Michael Brooks <mlbrooks@…>

Branches:

ADT, arm-eh, ast-experimental, enum, forall-pointer-decay, jacob/cs343-translation, master, new-ast, new-ast-unique-expr, pthread-emulation, qualifiedEnum

Children:

92a6e02

Parents:

ae265b55

Message:

Changing new AST's ImplicitCtorDtorStatement? to _own_ its reference to the underlying ctor/dtor call statement.

This change fixes a new-AST-resolver bug in which bootloader output lacks constructor/destructor calls surrounding overt declarations. (Constructor/destructor calls on temporaries were already working.)

This fix addresses the last bootloader output difference: now new-AST's bootloader output equals old AST's.

ImplicitCtorDtorStatement?'s reference to the underlying ctor/dtor call statement changes from readonly<...> to ptr<...>. Accordingly, the visitor framework changes, now proceeding across this reference. Old-AST deletes across this reference on ~ImplicitCtorDtorStatement? and old-AST's visitor framework proceeds across this reference. Old AST's declaration of this reference had an incorrect comment saying the reference was non-owning; adjusting this comment.

Future issues with the ownership details of this reference may occur, e.g. when trying to port FixInit? to new-AST. In the new-AST modeling, we need to ensure that re-parenting works properly, i.e. we don't delete in a transition state where the ref count falls to zero. The lifecycle of the underlying ctor/dtor statement is only partially understood. I believe, but have only partly tested, that for overt declarations:

• the call statement is generated on InitTweak? (before resolve)
• the call statement is swapped into a specific call site, away from c.callStmt, where c == o.init.ctor: ImplicitCtorDtorStatement?, o: ObjectDeclaration? (somewhere after resolve)
• the ImplicitCtorDtorStatement? gets deleted (somewhere after resolve)
• the old AST's ~ImplicitCtorDtorStatement? never runs a deletion of this.callStmt, because it is always null by that time

This change has been tested for a clean convert-convert difference (vs no new-AST at all), for convert-convert at: pre-resolve, post-resolve, post-parse.

This change has been tested for, and causes a small regression of, cfa-cpp speed of compiling bootloader. Old AST = 4.72 s, New AST before this change = 3.74 s, New ast after this change 3.79 s. All numbers are mean of middle 3, among 5 samples, with 5-sample SDs <= 0.16 sec and mid-3 SDs <= 0.012 sec. New-AST improvement before this fix = 20.8%, new-AST improvement after this fix = 19.7%, a loss of 5.3% of the improvement.

The performance loss makes sense because more constructor calls are being exposed to the resolver. Constructor calls are generally most expensive to resolve, because of their high number of function-name matches.

Location:

src/AST

Files:

• Pass.impl.hpp (modified) (1 diff)
• Stmt.hpp (modified) (1 diff)

src/AST/Pass.impl.hpp ¶

@@ -953 +953 @@
         // For now this isn't visited, it is unclear if this causes problem
         // if all tests are known to pass, remove this code
-        // VISIT(
-        //      maybe_accept( node, &ImplicitCtorDtorStmt::callStmt );
-        // )
+        VISIT(
+                maybe_accept( node, &ImplicitCtorDtorStmt::callStmt );
+        )
 
         VISIT_END( Stmt, node );

src/AST/Stmt.hpp ¶

@@ -399 +399 @@
 class ImplicitCtorDtorStmt final : public Stmt {
 public:
-        readonly<Stmt> callStmt;
+        ptr<Stmt> callStmt;
 
         ImplicitCtorDtorStmt( const CodeLocation & loc, const Stmt * callStmt,