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

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Sep 11 02:17:13 PDT 2019


jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-readobj/StackMapPrinter.h:25
+  // FIXME: There could be multiple entries of different versions in
+  // .llvm_stackmaps section. See: https://bugs.llvm.org/show_bug.cgi?id=38277
+  uint64_t Offset = 0x0;
----------------
jhenderson wrote:
> You can get rid of the ':' after "See" and add a trailing full stop.
> 
> I'd rephrase the first sentence as: "There could be multiple entries in a .llvm_stackmaps section, but this code only handles the first." The version issue isn't really the whole story here, so there's no need to specifically highlight it.
The trailing full stop should be after the hyperlink, not after the "See":

"See https://bugs.llvm.org/show_bug.cgi?id=38277."

(Actually, I see that Phabricator tries to add the '.' to the hyperlink (as well as the double quotes...). Not sure if this is specific to Phabricator or if other editors make this mistake too, so you might just want to get rid of the full stop altogether in this case).


================
Comment at: llvm/tools/llvm-readobj/StackMapPrinter.h:32
+                                      " detected at offset 0x" +
+                                      utohexstr(Offset, true /* Lower case */) +
+                                      ", which is not supported."),
----------------
jhenderson wrote:
> I think the usual pattern is `/*LowerCase=*/true` (where `LowerCase` is the parameter name).
Please use the exact formatting as in my previous comment, not as you have done. It's important to get right, because clang-format recognises this pattern (i.e. without the spaces).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67328





More information about the llvm-commits mailing list