[PATCH] D68634: [ASTImporter] Imported FunctionDecls inherit attributes from existing functions

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added a comment.

In D68634#1700591 <https://reviews.llvm.org/D68634#1700591>, @a_sidorin wrote:

> Hello Balasz,
>  In my opinion, importing and setting the attributes should be handled by the stuff used in InitializeImportedDecl(). Can we extend it or reuse the code? It will allow us not to miss the required actions while importing a new function too.


I was thinking about that too, but it turned out to be a way more intrusive change.

We have to work with the most recent existing decl (`PrevDecl`) of the FunctionDecl whose attribute we currently import.
We can get the `PrevDecl` only with the help of the lookup. We can't get it from the `ToD` param of `InitializeImportedDecl` because by the time when we call `InitializeImportedDecl` the redecl chain is not connected. So, we should pass `PrevDecl` then into `GetImportedOrCreateDecl`.  It would require to change all call expressions of `GetImportedOrCreateDecl` (58 occurences). Besides, we would use the `PrevDecl` only in case of inheritable attributes, only they require this special care. Note that there are a bunch of non-inheritable attributes which are already handled correctly in `InitializeImportedDecl`.



================
Comment at: clang/lib/AST/ASTImporter.cpp:7842
+// This implementation is inspired by Sema::mergeDeclAttributes.
+void ASTNodeImporter::CopyInheritedAttributes(FunctionDecl *Old,
+                                              FunctionDecl *New) {
----------------
shafik wrote:
> There are other attributes that [apply to functions as well](https://en.cppreference.com/w/cpp/language/attributes): `nodiscard`, `maybe_unused` and `deprecated`. Is there a reason not to support those as well?
Different attributes require different handling.

E.g. C++11 [[noreturn]] requires diagnostics if the first declaration of a function does not specify a the attribute, but a later does:
```
void f();
[[noreturn]] void f(); // ERROR
```
If a function is declared with [[noreturn]] in one translation unit, and the same function is declared without [[noreturn]] in another translation unit, the program is ill-formed; no diagnostic required.

But this is not true to the GNU __attribute__((noreturn)).

Also, there are non-inheritable attributes, in their case we do not have to copy anything from the existing most-recent-decl.

Thus, I think I am going to change the patch's name to indicate we deal only with `__attribute__((noreturn))` and with `analyzer_noreturn`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68634