[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

[Aaron Ballman via Phabricator via cfe-commits]

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