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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
