[PATCH] D109397: [AttributeList] Change indexes in AttributeList::AttrIndex

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a subscriber: tstellar.
rnk added a comment.

This seems release note worthy. This seems likely to affect every LLVM frontend. + at tstellar for some distributor PoV.

I think one of the reasons I held off on doing this was because I knew we'd have to do the conversion in the C API and the bitcode serialization, and that was two places that needed to know about the index conversion instead of one in the internals.



================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:791
     Record.push_back(VE.getAttributeGroupID(Pair));
-    Record.push_back(AttrListIndex);
+    // For historical reasons, the attribute index if off by one.
+    Record.push_back(AttrListIndex - 1);
----------------
typo s/if/is/


================
Comment at: llvm/lib/IR/Core.cpp:2464
+// API the same.
+static unsigned ConvertAttributeIndex(LLVMAttributeIndex Idx) {
+  return Idx + 1;
----------------
Re: the lint check, the static helpers in this file do start with lower case names. I also suggest making the directionality a bit clearer, like `mapToLlvmAttributeIndex` or something like that. We don't need mapFrom, but I was wondering that to myself.


================
Comment at: llvm/unittests/IR/AttributesTest.cpp:175
+      {AttributeList::FunctionIndex, Attribute::get(C, Attribute::ReadOnly)},
+      {AttributeList::ReturnIndex, Attribute::get(C, Attribute::SExt)}};
   AttributeList AL = AttributeList::get(C, Attrs);
----------------
Hm, this seems like an annoying API break that most frontends will discover only at runtime.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109397