[PATCH] D118476: [demangler] Fix new/delete demangling

[Nathan Sidwell via Phabricator via llvm-commits]

urnathan added inline comments.


================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:4821
+      NodeArray Inits = popTrailingNodeArray(InitsBegin);
+      return make<NewExpr>(ExprList, Ty, Inits, Global, IsArray);
+    }
----------------
ChuanqiXu wrote:
> urnathan wrote:
> > urnathan wrote:
> > > ChuanqiXu wrote:
> > > > In case `HaveInits` is false and `consumeIf('E')` return true, the original behavior would be `return make<NewExpr>(ExprList, Ty, NodeArray(), Global, IsArray);`. Is the current behavior expected?
> > > You're misreading the old code -- there's a negation on the non-looping consume( 'E').  There's always an 'E', it's just that without 'pi' that 'E'must be immediate (there are no expressions).
> > > 
> > > Here's the grammar -- you always reach an 'E'
> > > 
> > > // [gs] nw <expression>* _ <type> E                     # new (expr-list) type
> > > // [gs] nw <expression>* _ <type> <initializer>         # new (expr-list) type (init)
> > > // [gs] na <expression>* _ <type> E                     # new[] (expr-list) type
> > > // [gs] na <expression>* _ <type> <initializer>         # new[] (expr-list) type (init)
> > > // <initializer> ::= pi <expression>* E                 # parenthesized initialization
> > > 
> > > (Not sure why the 'pi' initializer introducer is needed, probably history.)
> > > 
> > > OLD:
> > > if ('pi') { while not 'E' {read expr; } build newexpr (exprs)
> > > else if not 'E'; fail
> > > else build newexpr ();
> > > 
> > > NEW:
> > > haveinits = 'pi';
> > > while not 'E' { if !haveinits; fail, else read expr }
> > > build newexpr (exprs)
> > > 
> > oh, of course 'pi' is needed as 'new int' and 'new int ()' are different.  Not sure why 'new int {}' is not representable, perhaps no one's got there yet?
> Yeah, the point here is if 'pi' is not true, then there must be an immediate 'E'. Then the code makes sense. It is a little surprising to me that the parser of demangler wouldn't handle the wrong case. Maybe the basic assumption is that the input is valid.
> 
> For the case of 'new int {}', I guess it is represented as there are multiple <expression> between pi and 'E'.
I'm not sure I follow about 'wouldn't handle the wrong case'.  The parser should definitely not assume the input is valid.  What invalid input have I missed?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118476/new/

https://reviews.llvm.org/D118476