[PATCH] D137095: [llvm-readobj] Output valid JSON for GroupSections

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Looks essentially good. Just a few nits and my ongoing comment about testing.



================
Comment at: llvm/test/tools/llvm-readobj/ELF/llvm-vs-json-format.test:414
 
+## Check that group sections are consistent between LLVM and JSON formats
+
----------------
As in the other cases, I think you're better off moving this test case into the standard test case for printing group sections.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:719-722
+  virtual void printEmptyGroupMessage() const;
+  virtual void printSectionGroupMembers(const StringRef Name,
+                                        uint64_t Idx) const;
+  virtual std::string getGroupSectionHeaderName() const;
----------------
I would consider reordering the order these methods are in. Specifically, `getGroupSectionHeaderName` is used before the others, so it should be first, and `printEmptyGroupMessage` is used last, so it should be last in order.

Also applies to the definition order and other places where the function overrides are declared.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:747-748
                            const unsigned SecNdx) override;
+  void printEmptyGroupMessage() const override;
+
+  void printSectionGroupMembers(const StringRef Name,
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137095