[PATCH] D75015: [ARM] Rewrite ARMAttributeParser

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Support/ARMAttributeParser.cpp:402
+  uint64_t pos;
+  uint64_t end = cursor.tell() - 5 + length;
+  while ((pos = cursor.tell()) < end) {
----------------
compnerd wrote:
> I think that the original code was clearer here.  I think at the very least, we should have a comment explaining this offset.
Moved `- 5` to the caller.


================
Comment at: llvm/lib/Support/ARMAttributeParser.cpp:417
+    if (!handled) {
+      if (tag < 32)
+        return createStringError(errc::invalid_argument,
----------------
compnerd wrote:
> Bleh, could you please check if we have a constant for this yet?  IIRC, we didn't have one when I wrote this, but really this is the limit of the namespace and should really have a constant.
Will do afterwards.


================
Comment at: llvm/lib/Support/ARMAttributeParser.cpp:447
+Error ARMAttributeParser::parseSubsection(uint32_t length) {
+  uint64_t end = cursor.tell() - 4 + length;
+  StringRef vendorName = de.getCStrRef(cursor);
----------------
compnerd wrote:
> Again, I think that the original code was more readable here.  Please use `sizeof(length)` or add a comment explaining the 4.
Moved `- 4` to the caller. I think it will be more readable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75015