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

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked an inline comment as done.
erichkeane added inline comments.


================
Comment at: cfe/trunk/include/clang/Basic/AttributeCommonInfo.h:166
+               ? SpellingIndex
+               : calculateAttributeSpellingListIndex();
+  }
----------------
plotfi wrote:
> erichkeane wrote:
> > aheejin wrote:
> > > MaskRay wrote:
> > > > 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)
> > > > ```
> > > +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.
> > 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.
> 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?
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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67368/new/

https://reviews.llvm.org/D67368