[PATCH] D123955: [C2x] Disallow functions without prototypes/functions with identifier lists

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/Basic/LangOptions.def:124
 BENIGN_LANGOPT(ImplicitInt, 1, 0, "C89 implicit 'int'")
+LANGOPT(StrictPrototypes  , 1, 0, "require function types to have a prototype")
 LANGOPT(Digraphs          , 1, 0, "digraphs")
----------------
This makes me think we should have some declarative way of specifying dependencies between `LANGOPT`s. It's presumably sufficiently obvious to a library user that they shouldn't enable (eg) `CPlusPlus20` unless they enable all the previous `CPlusPlusXY` modes and `CPlusPlus`, but I doubt it's obvious that enabling `CPlusPlus` requires also enabling `StrictPrototypes`.

In fact, after this change, I think a lot of existing library users of Clang that invent their own `LangOptions` will silently start getting this wrong. That's concerning. Maybe we should consider prototypes to be required if either `StrictPrototypes` or `CPlusPlus` is enabled?


================
Comment at: clang/lib/Parse/ParseDecl.cpp:6664-6666
+    // OpenCL disallows variadic functions, so it also disallows a function
+    // without a prototype. However, it doesn't enforce strict prototypes
+    // because it allows function definitions with an identifier list.
----------------
I don't follow this comment: functions without a prototype are not variadic (they're compatible with any *non-variadic* prototype), so OpenCL disallowing variadic functions seems irrelevant here.


================
Comment at: clang/lib/Sema/SemaType.cpp:5273-5275
+      // OpenCL disallows variadic functions, so it also disallows a function
+      // without a prototype. However, it doesn't enforce strict prototypes
+      // because it allows function definitions with an identifier list.
----------------
Same as before, OpenCL disallowing variadics doesn't seem relevant here.


================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1034-1042
+          if (T.isNull() || !T->getAs<FunctionType>())
+            // If the type is invalid or is not a function type, we cannot get
+            // a block pointer type for it. This isn't ideal, but it's better
+            // than asserting in getBlockPointerType() or creating a function
+            // without a prototype in a language that has no such concept (like
+            // C++ or C2x).
+            sReg = getUnknownRegion();
----------------
I find it really surprising that the "signature is present but is not a function type" case is reachable -- the static analyzer should only run on valid code, and in valid code I'd expect the signature of a block would always be a function type. Is that  case actually reached in our test suite?

I worry that the "block has no explicit signature" case here is common, and that we're losing substantial coverage in that case. Per https://clang.llvm.org/docs/BlockLanguageSpec.html#block-literal-expressions, `^ {  ...  }` is equivalent  to `^ (void) { ... }`, so it seems the original code here was just wrong and we should always have been creating a `FunctionProtoType`  in this case.


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

https://reviews.llvm.org/D123955