NoQ added inline comments.
================
Comment at: clang/test/Sema/warn-calls-without-prototype.c:39
+ return a + b +c;
+}
+
----------------
delcypher wrote:
> 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?
Ok dunno but I definitely find this whole thing surprising. I'd expect this
example to be the entirely normal situation for this code, where it sees that
the new declaration has no prototype so it "inherits" it from the old
declaration. But you're saying that
> `isa<FunctionNoProtoType>(NewFuncType)` is false in this particular case
Where does that proto-type come from then? I only see this code affecting the
type
```lang=c++
3523 if (RequiresAdjustment) {
3524 const FunctionType *AdjustedType =
New->getType()->getAs<FunctionType>();
3525 AdjustedType = Context.adjustFunctionType(AdjustedType,
NewTypeInfo);
3526 New->setType(QualType(AdjustedType, 0));
3527 NewQType = Context.getCanonicalType(New->getType());
3528 }
```
which doesn't seem to touch no-proto-types:
```lang=c++
3094 const FunctionType *ASTContext::adjustFunctionType(const FunctionType
*T,
3095
FunctionType::ExtInfo Info) {
3096 if (T->getExtInfo() == Info)
3097 return T;
3098
3099 QualType Result;
3100 if (const auto *FNPT = dyn_cast<FunctionNoProtoType>(T)) {
3101 Result = getFunctionNoProtoType(FNPT->getReturnType(), Info);
...
3107 }
3108
3109 return cast<FunctionType>(Result.getTypePtr());
3110 }
```
So it sounds like `New->getType()` was already with prototype from the start?
Maybe whatever code has set that type should also have set the
`HasInheritedPrototype` flag?
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
