[PATCH] D75131: [llvm-objdump][XCOFF][AIX] Implement -r option

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:252
+  static constexpr uint64_t InvalidRelocOffset =
+      std::numeric_limits<uint64_t>::max();
+
----------------
jasonliu wrote:
> jhenderson wrote:
> > hubert.reinterpretcast wrote:
> > > MaskRay wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > `0xffffffff'ffffffffULL`
> > > > Will UINT64_C(-1) be clearer?
> > > You mean `-UINT64_C(1)`? I don't think so. My experience is that people are not familiar with these macros.
> > I would use std::numeric_limits<uint64_t>::max(), since it's clearer to me than a literal. It's meaning seems obvious.
> This was my original implementation, I don't have anything against it.
> 
> @hubert.reinterpretcast Any comments?
I was hoping to not add an extra header inclusion to an interface file, but readability concerns are more important. @jasonliu, sorry for the churn.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:324
+  if (is64Bit())
+    report_fatal_error("64-bit support not implemented yet.");
+  const XCOFFSectionHeader32* SectionEntPtr = toSection32(Sec);
----------------
jhenderson wrote:
> hubert.reinterpretcast wrote:
> > jhenderson wrote:
> > > Here and all report_fatal_error calls: remove the trailing full stop.
> > We have been trying to full stops for XCOFF work for invocations of report_fatal_error, llvm_unreachable, and assert in order to be consistent within our patches and after having observed that such uses are not uncommon.
> > 
> > For example:
> > ```
> > llvm/lib/Object/COFFObjectFile.cpp:    report_fatal_error("Section was outside of section table.");
> > llvm/lib/Object/COFFObjectFile.cpp:      report_fatal_error("Aux Symbol data was outside of symbol table.");
> > llvm/lib/Object/MachOObjectFile.cpp:    report_fatal_error("Malformed MachO file.");
> > llvm/lib/Object/MachOObjectFile.cpp:    report_fatal_error("Requested symbol index is out of range.");
> > ```
> They may not be uncommon, because people haven't been particularly careful with how they write error messages in the past. Whereever possible, I've tried to encourage people to adopt a single consistent style - note that most (all?) of our error messages in clang, LLD and various tools like llvm-readobj and llvm-objdump use lower-case and no trailing full stop. report_fatal_error calls should be no different.
@jhenderson, I agree that a single consistent style would be great. I note that the Coding Standards do not document this style. It is also unfortunate that this style is not consistent, in respect to the capitalization of the first word, with the examples in the Coding Standards for strings in `assert` and `llvm_unreachable`. If you find consensus to update the Coding Standards to document this style, it would probably help to improve the consistency in the project.


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

https://reviews.llvm.org/D75131