[PATCH] D88390: [M68k] (Patch 4/8) MC layer and object file support

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

A few nits, but this is looking good.



================
Comment at: llvm/include/llvm/MC/MCExpr.h:202
     VK_PCREL,
+    VK_GOTPC,
     VK_GOTPCREL,
----------------
Does this get exposed in any way? Would it break a previous enum order?


================
Comment at: llvm/lib/Target/M68k/MCTargetDesc/M68kAsmBackend.cpp:216
+  for (uint64_t i = 0; i != NumNops; ++i) {
+    OS << "\x4E\x71";
+  }
----------------
Isn't there a better way to emit this?


================
Comment at: llvm/lib/Target/M68k/MCTargetDesc/M68kBaseInfo.h:1
+//===-- M68kBaseInfo.h - Top level definitions for M680X0 MC --*- C++ -*-===//
+//
----------------
Other header comments mention "m68k", this should too.


================
Comment at: llvm/lib/Target/M68k/MCTargetDesc/M68kBaseInfo.h:216
+  case M68k::A6:
+  // case M68k::A7:
+  case M68k::SP:
----------------
Is this comment really meaningful? Or is it commented by accident? 

Or is "SP" == "A7"? If so, I wouldn't comment like code because it looks like an accident or bad form.


================
Comment at: llvm/lib/Target/M68k/MCTargetDesc/M68kELFObjectWriter.cpp:12
+///
+//===----------------------------------------------------------------------===//
+
----------------
This file is quite light on comments.


================
Comment at: llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp:11
+/// This file contains defintions for M68k code emitter.
+///
+//===----------------------------------------------------------------------===//
----------------
This file is also a bit light on comments. Some assumptions may not be obvious to non-m68k developers.


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

https://reviews.llvm.org/D88390