[PATCH] D64811: Warn when NumParams overflows

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:6673
+  // Avoid exceeding the maximum function parameters
+  // See https://bugs.llvm.org/show_bug.cgi?id=19607
+  if (ParamInfo.size() > Type::getMaxNumParams()) {
----------------
Mordante wrote:
> rjmccall wrote:
> > rjmccall wrote:
> > > We don't usually cite bugs like this in the main source code except as "there's a bug with this, here's where we tracking fixing it".  The comment explains the purpose of the diagnostic well enough (but please include a period).
> > > 
> > > Can you just drop the excess arguments here instead of cutting off parsing?
> > Or actually — let's move this out of the parser entirely, because (unlike prototype scope depth) we actually need to diagnose it in Sema as well because of variadic templates.  `Sema::BuildFunctionType` seems like the right place.
> > 
> > Also, can you `static_assert` that function declarations support at least as many parameter declarations as we allow in types?
> I tried your suggestion to move the test to `Sema::BuildFunctionType` but that function is only used for templates so it doesn't show the warning for other cases. Also the error generated will also print the `note_ovl_candidate_substitution_failure` diagnostic making the output rather unreadable. So I'll look for a better way to handle the variadic template case.
> 
Hmm, yes, we should find a way to suppress follow-on errors like that.  But we do need to be able to avoid problems even when building parameter lists that don't correspond to a declared function, e.g. with `template <class... T> void foo(void (*fp)(T...));`.  (You'll need to pass the template arguments explicitly here, since we obviously can't infer an excessive number of parameters from a function whose parameter count is limited by the same constraint.)

And I always forget that function type parsing doesn't go through that function, sorry.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64811