[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.


================
Comment at: clang/test/Sema/warn-calls-without-prototype.c:39
+  return a + b +c;
+}
+
----------------
delcypher wrote:
> delcypher wrote:
> > @NoQ Any ideas about this?  It seems kind of weird that when merging `not_a_prototype3` prototype with the K&R style definition of `not_a_prototype3` that the resulting FunctionDecl we see at the call site in `call_to_function_without_prototype3` is marked as not having a prototype.
> > 
> > If I flip the order (see `not_a_prototype6`) then the merged declaration is marked as having a prototype.
> > 
> > I'm not sure if this is a bug in `Sema::MergeFunctionDecl` or if this just a peculiarity of K&R style function definitions.
> I suspect the problem might be here in `Sema::MergeFunctionDecl`.
> 
> ```lang=c++
>    // C: Function types need to be compatible, not identical. This handles
>   // duplicate function decls like "void f(int); void f(enum X);" properly.
>   if (!getLangOpts().CPlusPlus &&
>       Context.typesAreCompatible(OldQType, NewQType)) {
>     const FunctionType *OldFuncType = OldQType->getAs<FunctionType>();
>     const FunctionType *NewFuncType = NewQType->getAs<FunctionType>();
>     const FunctionProtoType *OldProto = nullptr;
>     if (MergeTypeWithOld && isa<FunctionNoProtoType>(NewFuncType) &&
>         (OldProto = dyn_cast<FunctionProtoType>(OldFuncType))) {
>       // The old declaration provided a function prototype, but the
>       // new declaration does not. Merge in the prototype.
> ```
> 
> ` isa<FunctionNoProtoType>(NewFuncType)` is false in this particular case, however `New` doesn't have a prototype (i.e. `New->hasPrototype()` is false). One fix might be to replace `isa<FunctionNoProtoType>(NewFuncType)` with 
> 
> ```
> (isa<FunctionNoProtoType>(NewFuncType) || !New->hasPrototype())
> ```
> 
> However, I don't really know this code well enough to know if that's the right fix.
Okay. I think the above would actually be the wrong location for a fix because in this case we don't need to go down the path that synthesizes the parameters because we already know them for both `old` and `new` in this situation.

Instead I think the change would have to be in `Sema::MergeCompatibleFunctionDecls` to do something like.

```lang=c++
  // If New is a K&R function definition it will be marked
  // as not having a prototype. If `Old` has a prototype
  // then to "merge" we should mark the K&R function as having a prototype.
  if (!getLangOpts().CPlusPlus && Old->hasPrototype() && !New->hasPrototype())
    New->setHasInheritedPrototype(); 
```

What I'm not sure about is if this is semantically the right thing to do. Thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116635