[PATCH] D58659: [Sema] Fix assertion when `auto` parameter in lambda has an attribute.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.


================
Comment at: clang/lib/Sema/SemaType.cpp:266
+        auto *NewAttrTy = cast<AttributedType>(T.getTypePtr());
+        for (TypeAttrPair &A : AttrsForTypes) {
+          if (A.first == AttrTy)
----------------
jkorous wrote:
> Do you think it would make sense to terminate the loop early if `AttrsForTypesSorted == true`?
I agree with you it would be nice to reuse `AttrsForTypesSorted` property but in practice it doesn't matter. We sort `AttrsForTypes` only in `takeAttrForAttributedType` and we should do that after we've populated `AttrsForTypes` and after we've replaced `auto` types. It isn't strictly required but that's how it was working in debugger. Also I've tried to add `assert(! AttrsForTypesSorted);` and it wasn't triggered during `ninja check-clang`

Having `AttrsForTypes` sorted can be useful in the future but I don't think we should add code for that now.

Despite the linear iteration, we are not pessimizing the most common use case with no attributes for lambda parameters (expression "most common use case" is based on my experience and not substantiated by any data). So I think the performance shouldn't degrade noticeably.


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

https://reviews.llvm.org/D58659