Page MenuHomePhabricator

[NFCI]Create CommonAttributeInfo Type as base type of *Attr and ParsedAttr.

Authored by erichkeane on Sep 9 2019, 1:20 PM.



In order to enable future improvements to our attribute diagnostics,
this moves info from ParsedAttr into CommonAttributeInfo, then makes
this type the base of the *Attr and ParsedAttr types. Quite a bit of
refactoring took place, including removing a bunch of redundant Spelling
Index propogation.

Diff Detail


Event Timeline

erichkeane created this revision.Sep 9 2019, 1:20 PM

Thank you for doing all this work -- in general, this is looking great and really cleans things up!

2266 ↗(On Diff #219419)

I'm surprised to see this as part of an NFC refactoring. Hopefully it will become more obvious later. :-D

66–69 ↗(On Diff #219419)

Do you want to do = 0 here to give them in-class initializers? (well, SpellingIndex should probably be SpellingNotCalculated).

373 ↗(On Diff #219419)

Can you correct the order in the comment to be the same as the order in the pair?

298–300 ↗(On Diff #219419)

I'm surprised we'd give a parse syntax to an implicit attribute. Wouldn't we rather note that these have no associated syntax or spelling?

892 ↗(On Diff #219419)

Spurious change?

8962–8965 ↗(On Diff #219419)

It would be nice if we didn't have to repeat information, if possible. The previous version of this didn't need to specify AT_NoReturn because the C11NoReturnAttr was sufficient to glean that information. It would be awesome if we could find a way to make this more succinct. Perhaps ConcreteAttr::Create(Context, Range, Syntax, Spelling = DefaultValue) or something along those lines?

13880 ↗(On Diff #219419)

Spurious formatting change? Same for the next three changes.

3349 ↗(On Diff #219419)

Can we use something other than UnknownAttribute here? That implies we don't know the spelling of the attribute name, which seems wrong.

4464–4465 ↗(On Diff #219419)

Spurious formatting change.

2414–2415 ↗(On Diff #219419)

I'll stop pointing out the formatting changes, but you should probably drop these from the patch.

50 ↗(On Diff #219419)

Thank you for this change. I have no idea how the original code made it through a code review.

3942 ↗(On Diff #219419)

Since you're touching this code already... can you rename Attr to not conflict with a type name? :-D

56 ↗(On Diff #219419)

Is this still needed?

4526 ↗(On Diff #219419)

Do we also need bump an AST binary format version number for this sort of change?

erichkeane marked 15 inline comments as done.

I think I've changed everything @aaron.ballman suggested. Despite the size of this patch, it actually removes ~1020 lines of code!

erichkeane marked an inline comment as done.Sep 12 2019, 10:00 AM
erichkeane added inline comments.
2266 ↗(On Diff #219419)

Yep, was a separate bug I found while going through things. I'll take it out and do it later.

66–69 ↗(On Diff #219419)

Bitfield in-class initializers are a C++2a feature.

aaron.ballman accepted this revision.Sep 13 2019, 9:00 AM
aaron.ballman marked an inline comment as done.

LGTM! Thank you for this refactoring -- it was large, but I think the results from it are really good.

66–69 ↗(On Diff #219419)

Details. Invent a time machine and add this to C++14. ;-)

This revision is now accepted and ready to land.Sep 13 2019, 9:00 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2019, 10:42 AM
plotfi added a subscriber: plotfi.Sep 13 2019, 11:14 AM
This comment was removed by plotfi.

Yep! I fixed it in r371876.

MaskRay added inline comments.

calculateAttributeSpellingListIndex is defined in clangSema. This can cause lib/ (-DBUILD_SHARED_LIBS=on) fail to link:

ld.lld: error: undefined symbol: clang::AttributeCommonInfo::calculateAttributeSpellingListIndex() const       
>>> referenced by AttributeCommonInfo.h:166 (../tools/clang/include/clang/Basic/AttributeCommonInfo.h:166)     
>>>               tools/clang/lib/AST/CMakeFiles/obj.clangAST.dir/AttrImpl.cpp.o:(clang::AttributeCommonInfo::getAttributeSpellingListIndex() const)
sebpop added a subscriber: sebpop.Sep 14 2019, 9:38 AM

I still see a link error on aarch64-linux on master:

/usr/bin/ld: tools/clang/lib/AST/CMakeFiles/obj.clangAST.dir/AttrImpl.cpp.o: in function `clang::AttributeCommonInfo::getAttributeSpellingListIndex() const':
/home/ubuntu/llvm-project/llvm/tools/clang/include/clang/Basic/AttributeCommonInfo.h:166: undefined reference to `clang::AttributeCommonInfo::calculateAttributeSpellingListIndex() const'
collect2: error: ld returned 1 exit status

I reverted locally this patch and it finishes building.

aheejin added inline comments.Sep 15 2019, 1:40 AM

+1 This fails builds with -DBUILD_SHARED_LIBS=ON. I tried to add clangSema as a dependent library to clangAST, but this creates several circular dependencies.

erichkeane marked an inline comment as done.Sep 15 2019, 5:28 PM
erichkeane added inline comments.

Thanks for the heads up. The solution will just end up being moving that function's definition. I'll submit a patch on monday. Thanks for the reproducer.

plotfi added inline comments.Sep 15 2019, 5:34 PM

Normally I’d suggest a revert, but I can see downstream some stuff with Swift and apinotes was not completely trivial to get merged in with this patch. Is simply moving the definition the right solution here btw?

I would have definitely preferred a revert much earlier, I guess it's too late now, but not having a build is really inconvenient.

erichkeane marked an inline comment as done.Sep 16 2019, 6:16 AM
erichkeane added inline comments.

Yep, exactly. I would prefer someone to just fix it (since it IS something quite trivial) rather than revert. This ends up being a massive change for the purposes of improving diagnostics here and downstream.

Typically it is considered somewhat fair to give the author time to fix a failure before reverting, and unfortunately this was discovered at 8pm on a Friday, so it seems longer than this would typically take.

Fixed in r371985.