sammccall added a subscriber: rsmith. sammccall added a comment. In D86936#2338664 <https://reviews.llvm.org/D86936#2338664>, @erichkeane wrote:
> Why did we select the 'bracket-depth' for this? The documentation on that > doesn't seem to be anything close to what this should be based on: Based on a discussion with @rsmith - the formal definition of pack expansion is that it expands to nested parenthesized expressions expressions. > Sets the limit for nested parentheses, brackets, and braces to N in blocks, > declarators, expressions, and struct or union declarations. > > The 256 default limit is REALLY small for a fold expression, particularly > since the instantiation depth limit is 1024 by default. I think this patch > needs to be changed to use the InstantiationDepth instead. @rjmccall, > thoughts? One motivation here is to limit exposure of tools to stack overflows. A modest increase in the stack size of DynTypedNode triggered stack overflows in various traversals on real code. While it's possible in principle to write traversals that use only data recursion (and I've also fixed the ones in clang itself that I can find), in practice a lot of code and tools rely on recursive traversals, so trivially creating arbitrarily deep ASTs without any limit is certain to break things with unclear responsibility. (Whereas this change places responsibility with boost numerics). FWIW, somewhere between 1024 and 2048 was the critical depth on the systems we looked at, so 1024 seems... plausible, but large. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86936/new/ https://reviews.llvm.org/D86936 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
