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.
Details
Diff Detail
Event Timeline
Thank you for doing all this work -- in general, this is looking great and really cleans things up!
clang/include/clang/Basic/Attr.td | ||
---|---|---|
2266 | I'm surprised to see this as part of an NFC refactoring. Hopefully it will become more obvious later. :-D | |
clang/include/clang/Basic/AttributeCommonInfo.h | ||
66–69 | Do you want to do = 0 here to give them in-class initializers? (well, SpellingIndex should probably be SpellingNotCalculated). | |
clang/include/clang/Lex/Preprocessor.h | ||
373 | Can you correct the order in the comment to be the same as the order in the pair? | |
clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp | ||
298–300 | 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? | |
clang/lib/Sema/SemaAttr.cpp | ||
892 | Spurious change? | |
clang/lib/Sema/SemaDecl.cpp | ||
8962–8965 | 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 | Spurious formatting change? Same for the next three changes. | |
clang/lib/Sema/SemaDeclCXX.cpp | ||
3349 | Can we use something other than UnknownAttribute here? That implies we don't know the spelling of the attribute name, which seems wrong. | |
clang/lib/Sema/SemaDeclObjC.cpp | ||
4464–4465 | Spurious formatting change. | |
clang/lib/Sema/SemaObjCProperty.cpp | ||
2414–2415 | I'll stop pointing out the formatting changes, but you should probably drop these from the patch. | |
clang/lib/Sema/SemaStmtAttr.cpp | ||
50 | Thank you for this change. I have no idea how the original code made it through a code review. | |
clang/lib/Sema/SemaType.cpp | ||
3942 | Since you're touching this code already... can you rename Attr to not conflict with a type name? :-D | |
clang/lib/Serialization/ASTReaderDecl.cpp | ||
56 | Is this still needed? | |
clang/lib/Serialization/ASTWriter.cpp | ||
4526 | Do we also need bump an AST binary format version number for this sort of change? |
I think I've changed everything @aaron.ballman suggested. Despite the size of this patch, it actually removes ~1020 lines of code!
LGTM! Thank you for this refactoring -- it was large, but I think the results from it are really good.
clang/include/clang/Basic/AttributeCommonInfo.h | ||
---|---|---|
66–69 | Details. Invent a time machine and add this to C++14. ;-) |
I think this change might be breaking builds: http://lab.llvm.org:8011/builders/clang-aarch64-linux-build-cache/builds/16888
cfe/trunk/include/clang/Basic/AttributeCommonInfo.h | ||
---|---|---|
166 ↗ | (On Diff #220143) | calculateAttributeSpellingListIndex is defined in clangSema. This can cause lib/libclangAST.so.10svn (-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) |
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.
cfe/trunk/include/clang/Basic/AttributeCommonInfo.h | ||
---|---|---|
166 ↗ | (On Diff #220143) | +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. |
cfe/trunk/include/clang/Basic/AttributeCommonInfo.h | ||
---|---|---|
166 ↗ | (On Diff #220143) | 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. |
cfe/trunk/include/clang/Basic/AttributeCommonInfo.h | ||
---|---|---|
166 ↗ | (On Diff #220143) | 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.
cfe/trunk/include/clang/Basic/AttributeCommonInfo.h | ||
---|---|---|
166 ↗ | (On Diff #220143) | 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. |
I'm surprised to see this as part of an NFC refactoring. Hopefully it will become more obvious later. :-D