[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

v.g.vassilev closed this revision. v.g.vassilev added a comment. Landed in aca191cce1c4dbab28a65cfe4caa6348e698a2b3 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83174/new/ https://reviews.llvm.org/D83174 _

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

aaron.ballman added a comment. In D83174#2230795 , @gargvaibhav64 wrote: > I updated the -triple option to x86_64-pc-windows-msvc (as it was more > consistent with current tests). I also updated the path to Unix style in the > -I option. Thank you for

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

gargvaibhav64 updated this revision to Diff 287060. gargvaibhav64 added a comment. I updated the -triple option to x86_64-pc-windows-msvc (as it was more consistent with current tests). I also updated the path to Unix style in the -I option. CHANGES SINCE LAST ACTION https://reviews.llvm.org

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

JamesNagurne added a comment. In D83174#2230763 , @aaron.ballman wrote: > Unfortunately, I had to revert the change as it was causing some buildbots to > fail. I reverted in 58c305f466d1f78adb10e7295b9bc9fc192a6e09 >

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

aaron.ballman added a comment. Unfortunately, I had to revert the change as it was causing some buildbots to fail. I reverted in 58c305f466d1f78adb10e7295b9bc9fc192a6e09 . Some failures include: http://lab.llvm.org:8011/build

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

aaron.ballman added a comment. In D83174#2230616 , @gargvaibhav64 wrote: > In D83174#2230546 , @aaron.ballman > wrote: > >> LGTM, thank you for the fix! Do you need someone to commit on your behalf? >> If so, ple

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

gargvaibhav64 added a comment. In D83174#2230546 , @aaron.ballman wrote: > LGTM, thank you for the fix! Do you need someone to commit on your behalf? If > so, please be sure you're fine with the license agreement and let us know > what name and email ad

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, thank you for the fix! Do you need someone to commit on your behalf? If so, please be sure you're fine with the license agreement and let us know what name and email addres

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

gargvaibhav64 updated this revision to Diff 287004. gargvaibhav64 marked an inline comment as done. gargvaibhav64 added a comment. Updated the regex to be less-greedy. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83174/new/ https://reviews.llvm.org/D83174 Files: clang/lib/Serializat

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

aaron.ballman added inline comments. Comment at: clang/test/Modules/Inputs/inherit-attribute/b.h:3 + +class Foo; + Does this forward declaration impact the test? I can't quite tell whether this exists to test that the attribute is attached to the non-defining re

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

gargvaibhav64 updated this revision to Diff 286786. gargvaibhav64 added a comment. The test is now working properly. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83174/new/ https://reviews.llvm.org/D83174 Files: clang/lib/Serialization/ASTReaderDecl.cpp clang/test/Modules/Inputs/i

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

martong added a comment. In D83174#2158275 , @aaron.ballman wrote: > In D83174#2156454 , @v.g.vassilev > wrote: > > > >> Also, I would like to add that the current test present in this diff > > >> does not fail i

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

aaron.ballman added a comment. In D83174#2174294 , @gargvaibhav64 wrote: > Hi everyone, are there any more changes required on this review? FWIW, I'm holding my LG until there's a test case which covers the changes. CHANGES SINCE LAST ACTION https:/

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

gargvaibhav64 added a comment. Hi everyone, are there any more changes required to this review? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83174/new/ https://reviews.llvm.org/D83174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

aaron.ballman added reviewers: aaron.ballman, riccibruno, martong. aaron.ballman added a comment. Herald added a subscriber: rnkovacs. In D83174#2156454 , @v.g.vassilev wrote: > >> Also, I would like to add that the current test present in this diff does

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

v.g.vassilev removed a reviewer: aaron.ballman. v.g.vassilev added a comment. >> Also, I would like to add that the current test present in this diff does >> not fail in the existing system but it was the best I and Vassil could come >> up with to replicate our problem. > > Is there a way to

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

aaron.ballman added a comment. In D83174#2155143 , @gargvaibhav64 wrote: > The tests weren't failing for me. So, we are possibly missing test coverage. > I have made the change now. Thanks for making the change. > Also, I would like to add that the cu

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

gargvaibhav64 updated this revision to Diff 278399. gargvaibhav64 added a comment. The tests weren't failing for me. So, we are possibly missing test coverage. I have made the change now. Also, I would like to add that the current test present in this diff does not fail in the existing system b

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

aaron.ballman added inline comments. Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3544 +const auto *IA = +dyn_cast(Previous->getAttr()); + aaron.ballman wrote: > Don't do `hasAttr` followed by `getAttr` (that duplicates work); also, you > do

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

gargvaibhav64 updated this revision to Diff 277022. gargvaibhav64 marked 3 inline comments as done. gargvaibhav64 added a comment. Incorporated the changes CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83174/new/ https://reviews.llvm.org/D83174 Files: clang/lib/Serialization/ASTReade

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

aaron.ballman added a comment. In 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?

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

gargvaibhav64 updated this revision to Diff 276680. gargvaibhav64 marked 3 inline comments as done. gargvaibhav64 added a comment. Added a new attribute instead of using the previous one CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83174/new/ https://reviews.llvm.org/D83174 Files: c

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

gargvaibhav64 updated this revision to Diff 276351. gargvaibhav64 edited the summary of this revision. gargvaibhav64 added a comment. I incorporated the changes CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83174/new/ https://reviews.llvm.org/D83174 Files: clang/lib/Serialization/AST

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

rsmith added a comment. @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? Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3540 +Attr *I

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

gargvaibhav64 created this revision. gargvaibhav64 added reviewers: rsmith, v.g.vassilev. Herald added a project: clang. Herald added a subscriber: cfe-commits. This commit attaches teaches ASTDeclReader::attachPreviousDecl to successfully merge two Decl's when one contains an inheritable attribu