aaron.ballman added a comment.
In D83174#2137133 <https://reviews.llvm.org/D83174#2137133>, @rsmith wrote:
> @aaron.ballman We will need to do something like this in general, but I'm not
> sure it's going to be as easy as just inheriting the attribute in the general
> case. What do you think?
I agree that we're going to need a more general solution at some point. We do
this automatically in `mergeDeclAttributes()` with:
else if (Attr->shouldInheritEvenIfAlreadyPresent() || !DeclHasAttr(D, Attr))
NewAttr = cast<InheritableAttr>(Attr->clone(S.Context));
for the general case (and similar for merging parameter attributes in
`mergeParamDeclAttributes()`, but we have custom merging logic for the other
cases. However, the custom merging logic is usually for diagnosing bad
combinations of attributes, which I don't think we need to handle in this case,
do we?
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3544
+ const auto *IA =
+ dyn_cast<MSInheritanceAttr>(Previous->getAttr<MSInheritanceAttr>());
+
----------------
Don't do `hasAttr` followed by `getAttr` (that duplicates work); also, you
don't need to `dyn_cast<>` the call to `getAttr`. How about:
```
const auto *IA = Previous->getAttr<MSInheritanceAttr>();
if (IA && Previous->hasAttr<MSInheritanceAttr>()) {
...
}
```
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3546
+
+ NewAttr = new (Context) MSInheritanceAttr(
+ IA->getRange(), Context, IA->getBestCase(),
IA->getSpellingListIndex());
----------------
Rather than do it this way, how about: `NewAttr =
cast<InheritableAttr>(IA->clone(Context));`
================
Comment at: clang/test/Modules/Inputs/inherit-attribute/a.h:11
+#endif
\ No newline at end of file
----------------
Please add a newline to the end of the file. (Same for the other files.)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83174/new/
https://reviews.llvm.org/D83174
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
