Changeset c570806

Aug 15, 2019, 11:45:56 AM (5 years ago)
Michael Brooks <mlbrooks@…>
ADT, arm-eh, ast-experimental, enum, forall-pointer-decay, jacob/cs343-translation, master, new-ast, new-ast-unique-expr, pthread-emulation, qualifiedEnum

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:

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.

3 edited


  • src/AST/Pass.impl.hpp

    rae265b55 rc570806  
    953953        // For now this isn't visited, it is unclear if this causes problem
    954954        // if all tests are known to pass, remove this code
    955         // VISIT(
    956         //      maybe_accept( node, &ImplicitCtorDtorStmt::callStmt );
    957         // )
     955        VISIT(
     956                maybe_accept( node, &ImplicitCtorDtorStmt::callStmt );
     957        )
    959959        VISIT_END( Stmt, node );
  • src/AST/Stmt.hpp

    rae265b55 rc570806  
    399399class ImplicitCtorDtorStmt final : public Stmt {
    401         readonly<Stmt> callStmt;
     401        ptr<Stmt> callStmt;
    403403        ImplicitCtorDtorStmt( const CodeLocation & loc, const Stmt * callStmt,
  • src/SynTree/Statement.h

    rae265b55 rc570806  
    502502class ImplicitCtorDtorStmt : public Statement {
    503503  public:
    504         // Non-owned pointer to the constructor/destructor statement
     504        // the constructor/destructor call statement; owned here for a while, eventually transferred elsewhere
    505505        Statement * callStmt;
Note: See TracChangeset for help on using the changeset viewer.