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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 5 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:12
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_OBJECTYAML_XCOFFYAML_H
+#define LLVM_OBJECTYAML_XCOFFYAML_H
----------------
sfertile wrote:
> blank line before the ifndef.
done,thanks


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:121
+bool XCOFFObjectFile::isSectionCompressed(DataRefImpl Sec) const {
+  bool ret = false;
+  llvm_unreachable("Not yet implemented!");
----------------
hubert.reinterpretcast wrote:
> Minor nit: Naming is not consistent (`ret` versus `Result`).
change to Result


================
Comment at: llvm/lib/ObjectYAML/XCOFFYAML.cpp:17
+#include <cstdint>
+#include <vector>
+
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > I am not seeing a need for `<cstdint>` or `<vector>`. At the same time, `<string.h>` should be added for `::memset`.
> I deleted the 
> -#include "llvm/ObjectYAML/YAML.h"
> -#include "llvm/Support/YAMLTraits.h"
> -#include <cstdint>
> -#include <vector>
I think <string.h> is already be included in the headers files which be included in #include "llvm/ObjectYAML/XCOFFYAML.h" ,and I tried to use -E of g++ to preprocessor the file , I can find the string.h in the preprocessor file.


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

https://reviews.llvm.org/D59419