[PATCH] D59419: [XCOFF] Add functionality for parsing AIX XCOFF object files header .

[Sean Fertile via Phabricator via llvm-commits]

sfertile accepted this revision.
sfertile marked an inline comment as done.
sfertile added a comment.
This revision is now accepted and ready to land.

Other the a few minor nit picks, this LGTM.



================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:36
+
+struct XCOFFFileHeader {
+  support::ubig16_t Magic;
----------------
DiggerLin wrote:
> rnk wrote:
> > sfertile wrote:
> > > Rather then defining these in the `llvm::object` namespace why not drop the `XCOFF` part of the name and make the defintion nested inside `XCOFFObjectFile`?
> > (Not to bikeshed too much...) Personally, I'd leave this as is. It's consistent with the other object formats. It's useful to be able to do `using namespace llvm::object;`, but you cannot shorten `XCOFFObjectFile::Header` without a typedef or type alias.
> I am agreed with Reid Kleckner.
Fair enough.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:108
+} // namespace object
+} // namespace llvm
+#endif // LLVM_OBJECT_XCOFFOBJECTFILE_H
----------------
minor nit: add a blank line before the `endif`


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:12
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_OBJECTYAML_XCOFFYAML_H
+#define LLVM_OBJECTYAML_XCOFFYAML_H
----------------
blank line before the ifndef.


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:19
+namespace llvm {
+
+namespace XCOFFYAML {
----------------
minor nit: no blank line between the 2 namespace openings.


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:49
+} // namespace yaml
+} // namespace llvm
+#endif // LLVM_OBJECTYAML_XCOFFYAML_H
----------------
blank again.


================
Comment at: llvm/lib/ObjectYAML/XCOFFYAML.cpp:20
+namespace llvm {
+
+namespace XCOFFYAML {
----------------
nit: remove white-space.


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

https://reviews.llvm.org/D59419