[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:
> > 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?
> I mean the assumption: "There's always an 'E'. And without 'pi' that 'E' must be immediate". I think we get the conclusion from the grammar. But my question is "what if the input doesn't follow the grammar". Like there is not an immediate 'E' if 'p' is not presented. 
Yes, the grammar at this point permits either 'E' or 'pi' <expressions>* 'E' to appear.  If we don't get 'pi' then HaveInits will be false.  If we then don't get 'E' we'll enter the loop, and that will return nullptr as HaveInits is false.  I don;t see a problem.


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

https://reviews.llvm.org/D118476