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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

Thanks for working on this, Just a few comments



================
Comment at: clang/lib/Basic/LangOptions.cpp:119
   Opts.Digraphs = Std.hasDigraphs();
+  Opts.StrictPrototypes = Std.isCPlusPlus() || Std.isC2x();
 
----------------
Stupid question, we probably don't want -fno-strict-prototype` to have an effect in  C++. is there something to deal with that? I'm not familiar with the option system


================
Comment at: clang/lib/Sema/SemaDecl.cpp:8821-8823
+    assert(SemaRef.getLangOpts().StrictPrototypes
+               ? HasPrototype
+               : (true && "Strict prototypes are required"));
----------------
erichkeane wrote:
> This assert is quite strange, particularly with the string part of it only in the 'else' of the conditional? It seems you're trying to ensure that:
> "if strictProtoTypes, assert hasProtoType"?
> 
> In that case, I'd likely suggest:
> 
> `assert((HasProtoType || !SemeaRef.getLangOpts().StrictPrototypes) && "Strict prototypes are required");`
> 
> 
That would make more sense to me, not that it affects the logic
```
assert((SemaRef.getLangOpts().StrictPrototypes
               ? HasPrototype
               : true) && "Strict prototypes are required");
```


================
Comment at: clang/lib/Sema/SemaType.cpp:5299
+                  ? Context.getFunctionNoProtoType(T, EI)
+                                         : Context.IntTy;
           break;
----------------
can you explain the `Context.IntTy` here ?


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

https://reviews.llvm.org/D123955