[PATCH] D64825: Enable assembly output of local commons for AIX

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/trunk/include/llvm/MC/MCStreamer.h:547
+  virtual void EmitXCOFFLocalCommonSymbol(MCSymbol *Symbol, uint64_t Size,
+                             unsigned ByteAlignment);
+
----------------
Spacing issue.


================
Comment at: llvm/trunk/lib/MC/MCAsmStreamer.cpp:762
 
+// We need an XCOFF specific version of this directive as the AIX syntax
+// requires a QualName argument identifying the csect name and storage mapping
----------------
Minor nit: Compound adjective "XCOFF-specific" with a hyphen.


================
Comment at: llvm/trunk/lib/MC/MCAsmStreamer.cpp:768
+  assert(MAI->getLCOMMDirectiveAlignmentType() == LCOMM::Log2Alignment &&
+         "We only support writing log base-2 alignment format with XCOFF");
+  assert(isPowerOf2_32(ByteAlignment) && "alignment must be a power of 2");
----------------
Minor nit: Have a period at the end of sentences.


================
Comment at: llvm/trunk/lib/MC/MCAsmStreamer.cpp:769
+         "We only support writing log base-2 alignment format with XCOFF");
+  assert(isPowerOf2_32(ByteAlignment) && "alignment must be a power of 2");
+
----------------
Minor nit: Capitalize the first word of a sentence.


================
Comment at: llvm/trunk/lib/MC/MCSectionXCOFF.cpp:34
+        getMappingClass() != XCOFF::XMC_BS)
+      llvm_unreachable("Generated a storage-mapping class for a common/bss "
+                       "csect we don't understand how to switch to.");
----------------
I'm not sure that the "unreachables" (in the function currently, not just this one) are buying us enough to warrant the overhead of thinking about them. Generically, the `llvm_unreachable`s in this function are of the type "we are about to do something, and given the check, what we are about to do is not compatible with the input". We believe this to be a error in the internal state of the program that we are not expecting to happen, so `assert` is okay.


================
Comment at: llvm/trunk/lib/MC/MCXCOFFStreamer.cpp:63
+                                            unsigned ByteAlign) {
+  llvm_unreachable("Not implemented yet.");
+}
----------------
"Not implemented" is not a case where `llvm_unreachable` is appropriate. Either nothing calls this and it might as well be `report_fatal_error`, or something does call this and it is probably not the caller's fault that this is unimplemented (as it should be `report_fatal_error`).


================
Comment at: llvm/trunk/lib/MC/MCXCOFFStreamer.cpp:65
+}
\ No newline at end of file

----------------
Missing newline at end of file. Windows editor woes?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64825