nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.
LG from my side, but I'd like a second opinion for the "require LLVMContext in
AttrBuilder" part of the change, as that's the main API impact.
================
Comment at: llvm/lib/AsmParser/LLParser.cpp:136
const std::vector<unsigned> &Attrs = RAG.second;
- AttrBuilder B;
+ AttrBuilder B(M->getContext());
----------------
These `M->getContext()` can be replaced by just `Context`, as LLParser stores a
reference to that.
================
Comment at: llvm/lib/AsmParser/LLParser.cpp:1246
+ if (R == NumberedAttrBuilders.end())
+ R = NumberedAttrBuilders.emplace(VarID,
AttrBuilder(M->getContext())).first;
+
----------------
Is the separate find call here to avoid constructing AttrBuilder if it's
already in the map?
I was going to suggest that you can write this as `R =
NumberedAttrBuilders.emplace(VarID, M->getContext()).first;` and leave the
construction of `AttrBuilder` to emplace(), but it turns out that emplace()
still unconditionally constructs the object (because C++) and the sensible
replacement for this (`try_emplace`) has only been introduced in C++17.
================
Comment at: llvm/lib/IR/Attributes.cpp:1791
for (const auto &I : AM.td_attrs())
- TargetDepAttrs.erase(I);
+ removeAttribute(I);
----------------
We might as well directly resolve this TODO, I think it should be as simple as:
```
erase_if(TargetDepAttrs, [&](Attribute A) { return AM.contains(A); });
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116599/new/
https://reviews.llvm.org/D116599
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
