[PATCH] D67328: [llvm-readobj] Warn user when dumping not supported version of stack map (PR38278).

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/StackMapParser.h:325
 
-  /// Get the version number of this stackmap. (Always returns 3).
-  unsigned getVersion() const { return 3; }
+  /// Get the version number that this parser currently supported. (Always
+  /// returns 3).
----------------
supported -> supports

Maybe this should return an array of versions, and be called "getSupportedVersions"? Obviously in the first case, the array has only one member, but it would make it easy to add more in the future.


================
Comment at: llvm/test/tools/llvm-readobj/elf-stackmap-version2.test:1-2
+## Check that llvm-readobj/llvm-readelf reports a warning when dumping unsupported version
+## of stack map.
+
----------------
I'd change the last bit of this comment to:

"... reports a warning when parsing an unsupported stack map version."


================
Comment at: llvm/tools/llvm-readobj/StackMapPrinter.h:29-30
+                      "stack map entry with version " + Twine(Version) +
+                      " detected at offset " + Twine(0) + ". Only version " +
+                      Twine(SupportedVersion) + " is supported"),
+                  Filename);
----------------
If you accept my suggestion of returning multiple supported versions, you'll need to change this message again (sorry!). I'd suggest changing the second sentence to "Supported version(s): " and then simply list the array members.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67328