sepavloff marked 2 inline comments as done.
sepavloff added a comment.
In https://reviews.llvm.org/D30170#761342, @rsmith wrote:
> Do we really need two different notions of "definition" and "odr definition"
> here? What goes wrong if we always treat the "instantiation of a friend
> function definition" case as being a definition?
It requires 3 additional changes besides update of `isDefined`. All of them
implement current behavior of the function and look like:
@@ -3749,7 +3750,8 @@ void Sema::InstantiateFunctionDefinition(SourceLocation
PointOfInstantiation,
bool Recursive,
bool DefinitionRequired,
bool AtEndOfTU) {
- if (Function->isInvalidDecl() || Function->isDefined())
+ const FunctionDecl *Def;
+ if (Function->isInvalidDecl() || (Function->isDefined(Def) &&
Def->isThisDeclarationADefinition()))
return;
// Never instantiate an explicit specialization except if it is a class
scope
It make sense to replace this weird expression `Function->isDefined(Def) &&
Def->isThisDeclarationADefinition()` with a call to a new function, something
like `isActuallyDefined`. So we anyway have to add a new function for checking
function definition. In the case of `IsOdrDefinition` the new function is used
in one place only, modified version of `isDefined` requires changes in
unrelated code places.
Use of a special variant of `isDefined` dedicated to redefinition checks looks
less invasive and more robust. If however there are reasons to use modified
`isDefined`, it also can be implemented, there is nothing wrong with such
solution.
================
Comment at: include/clang/AST/Decl.h:1829-1830
+ /// Returns true if the function is defined and other definitions are not
+ /// allowed in the redeclaration chain.
+ ///
----------------
rsmith wrote:
> This will not be true in the presence of modules: we will ensure that at most
> one declaration has a body, but multiple declarations can be instantiated
> from a defined member function of a class template, for instance. So I think
> the constraint is that you're not allowed to add *more* such definitions.
Fixed also some comments according to this note.
================
Comment at: include/clang/AST/Decl.h:1848
+ return true;
+ if (FunctionDecl *Original = getInstantiatedFromMemberFunction())
+ return Original->isOdrDefined();
----------------
rsmith wrote:
> I think this will do the wrong thing for an explicit specialization of a
> member of a class template:
>
> ```
> template<typename T> struct A { void f() {}; };
> template<> void A<int>::f();
> ```
>
> Here, `A<int>::f()` is "instantiated from a member function" but it's not a
> definition, not even in the ODR sense. I think it would suffice to also check
> whether `Original` is a friend declaration here.
Although this function is not called in this case, its name must agree with its
functionality, so changed implementation as you recommend.
================
Comment at: lib/AST/Decl.cpp:2538
+ if (I->isThisDeclarationADefinition()) {
+ Definition = I->IsDeleted ? I->getCanonicalDecl() : I;
+ return true;
----------------
rsmith wrote:
> It seems incorrect to me for `isThisDeclarationADefinition()` to return
> `true` on a redeclaration of a deleted function: only the first declaration
> should be considered to be a definition in that case. (This special case
> should be in `isThisDeclarationADefinition()`, not in `isDefined()`.)
Indeed, if a declaration was selected to be a definition, it is strange if
`isThisDeclarationADefinition` returns false for it.
================
Comment at: lib/Sema/SemaDecl.cpp:11882
+
+ if (FD == Definition)
+ return;
----------------
rsmith wrote:
> This looks wrong to me: `CheckForFunctionRedefinition` is intended to be
> called *before* marking the function in question as a definition. If you call
> it afterwards, then the `isOdrDefined` call won't work properly because it
> will always check to see whether `FD` is a definition before checking any
> other declaration. So any case that reaches here with `FD` being a definition
> seems like a bug. I expect we're only getting here in that case due to the
> missing check for friend in `isThisDeclarationAnOdrDefinition`.
`CheckForFunctionRedefinition` is called after the declaration is put to
redeclaration chain. At this point the declaration does not have a body, so
problem may arise only if the provided declaration is an instantiation. It
happens when the method is called from `PerformPendingInstantiations` ->
`InstantiateFunctionDefinition` -> `ActOnStartOfFunctionDef` ->
`CheckForFunctionRedefinition`. In this case function body is generated for
declaration that is already in redeclaration chain. If the chain is in valid
state, it does not have duplicate definitions and call to
`CheckForFunctionRedefinition` is redundant and this check makes shortcut
return.
https://reviews.llvm.org/D30170
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
