[PATCH] D130331: [C++20] [Modules] Disable preferred_name when writing a C++20 Module interface
Tom Honermann via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Jul 22 09:38:38 PDT 2022
tahonermann added inline comments.
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:2925-2926
for (unsigned I = 0, E = readInt(); I != E; ++I)
- Attrs.push_back(readAttr());
+ if (auto *Attr = readAttr())
+ Attrs.push_back(Attr);
}
----------------
Interesting, this looks like a pre-existing issue. It looks like `readBTFTypeTagAttr()` in `clang/include/clang/Serialization/ASTRecordReader.h` has an assumption that `readAttr()` always returns non-null. I wonder if that should be modified to use `cast_or_null` rather than `cast`. I don't see any need to address that issue (if it actually exists) in this review though.
================
Comment at: clang/lib/Serialization/ASTWriter.cpp:4353
+ // https://github.com/llvm/llvm-project/issues/56490 for example.
+ if (!A || (isa<PreferredNameAttr>(A) && Writer->isWritingNamedModules()))
return Record.push_back(0);
----------------
ChuanqiXu wrote:
> The `Writer->isWritingNamedModules()` part is necessary. Otherwise we would break the https://github.com/llvm/llvm-project/blob/main/clang/test/PCH/decl-attrs.cpp test. The reason why the bug is not found by the user of PCH or clang modules is that a header generally would be guarded by `#ifndef ... #define` fashion. And if we remove the guard, the compiler would emit an error for duplicated definition. So the problem only lives in C++20 Named Modules.
Correct me if I'm mistaken, but I think this issue only occurs because, in the test, both modules have the problematic declarations in the global module fragment; thus creating duplicate definitions that have to be merged which then exposes the ODR mismatch.
I'm suspicious that this actually fixes all possible scenarios. For example:
//--- X1.cpp
#include "foo.h"
import A;
//--- X2.cpp
import A;
#include "foo.h"
I would expect the proposed change to cause an ODR issue in these scenarios since the definition from the module still needs to be merged in non-modular TUs, but now the imported module will lack the attribute present in the non-modular TUs.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130331/new/
https://reviews.llvm.org/D130331
More information about the cfe-commits
mailing list