[PATCH] D154921: Support -frecord-command-line for XCOFF integrated assembler path

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

Thank you for making the changes!

One small nit: I prefer using the implicit conversion to `bool` rather than explicitly checking for `nullptr`



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:605-607
+  std::unique_ptr<CInfoSymInfo> &CISI = CInfoSymSection.Entry;
+  if (CInfoSymSection.Entry != nullptr && nameShouldBeInStringTable(CISI->Name))
+    Strings.add(CISI->Name);
----------------



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1033
 void XCOFFObjectWriter::writeSectionHeaderTable() {
+  if (CInfoSymSection.Entry != nullptr)
+    writeSectionHeader(&CInfoSymSection);
----------------



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1091
 
+  if (CInfoSymSection.Entry != nullptr)
+    writeSymbolEntry(CInfoSymSection.Entry->Name, CInfoSymSection.Entry->Offset,
----------------



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1217
   // Calculate the file offset to the section data.
+  if (CInfoSymSection.Entry != nullptr) {
+    CInfoSymSection.FileOffsetToData = RawPointer;
----------------



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1314
+void XCOFFObjectWriter::addCInfoSymEntry(StringRef Name, StringRef Metadata) {
+  assert(CInfoSymSection.Entry == nullptr &&
+         "Multiple entries are not supported");
----------------



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1332
+
+  if (CInfoSymSection.Entry != nullptr) {
+    CInfoSymSection.Index = SectionIndex++;
----------------



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1593
+    CInfoSymSectionEntry &CInfoSymEntry, uint64_t &CurrentAddressLocation) {
+  if (CInfoSymSection.Entry == nullptr)
+    return;
----------------



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:258
+
+  CInfoSymSectionEntry(StringRef N, int32_t Flags) : SectionEntry(N, Flags){};
+  virtual ~CInfoSymSectionEntry() = default;
----------------
Jake-Egan wrote:
> scott.linder wrote:
> > formatting
> Do you mean that `CInfoSymSectionEntry(StringRef N, int32_t Flags) : SectionEntry(N, Flags){};` should be split into two lines? It seems git-clang-format wants it on one line.
Ah, my eyes just kind of glazed over the `;`, which is what seems to be causing `clang-format` to delete the space between the closing parens and the `{`. Sorry for the confusion!

I didn't realize it was even legal in this case (i.e. a semicolon after a method/constructor definition, not just a declaration), but it seems to just be an optional part of the grammar.

I'd suggest deleting the `;` and re-running `clang-format`, but I don't actually know that there is any policy on it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154921