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

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
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:
> > jasonliu wrote:
> > > MaskRay wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > 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.
> > > > Please stop capitalizing and stop using a trailing full stop. For the common infrastructure of LLVMObject and binary utilities where James and several other contributors spending a lot of time on, we have made lots of efforts improving the diagnostics and their consistency. As a relatively new user of these infrastructure, it'd be much appreciated if XCOFF could follow suite. I hope this XCOFF development in Object won't be sort of code dump.
> > > We are good with the consistent style. And I will make the change. We are just asking if it make sense to update Coding Standards to document the style as right now it's not all consistent through out the project, and it's easy to follow the wrong precedent. 
> > > >  I hope this XCOFF development in Object won't be sort of code dump.
> > > I don't find this comment to be very constructive. 
> > > 
> > > 
> > I believe that this is the first patch for which we were told about this convention. No one had said we were not changing this after James indicated that he believes it to be an important point. I find the insinuation that a "code dump" is being attempted to be rather extreme.
> Thanks for making the change. I'll post a question on llvm-dev about whether we should update the coding standard. I guess `assert` and `llvm_unreachable` are a little different, because of how they are printed. I'll do some investigation and write a full proposal up.
Thanks for the effort. Appreciate it. 


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

https://reviews.llvm.org/D75131