[PATCH] D57500: [WebAssembly] clang-tidy (NFC)

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

Generally LGTM.  I requested holding back on a few change.  Hope this makes sense.



================
Comment at: lib/MC/MCSectionWasm.cpp:17
 
-MCSectionWasm::~MCSectionWasm() {} // anchor.
-
----------------
I guess you are removing this anchor because we have other methods below?   


================
Comment at: lib/MC/WasmObjectWriter.cpp:1318
 
-        if (WS.getSize() == 0)
+        if (WS.getSize() == nullptr)
           report_fatal_error(
----------------
This looks wrong.. unless size is some kind of struct?


================
Comment at: lib/Object/WasmObjectFile.cpp:1352
 #undef ECase
-  return std::error_code();
+  return {};
 }
----------------
I prefer the old version here.


================
Comment at: lib/Object/WasmObjectFile.cpp:1373
                   S.Content.size());
-  return std::error_code();
+  return {};
 }
----------------
Revert this?


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:83
 
-  ~WebAssemblyOperand() {
+  ~WebAssemblyOperand() override {
     if (isBrList())
----------------
Can you explain this change?


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:278
       error("Expected identifier, got: ", Lexer.getTok());
-      return StringRef();
+      return {};
     }
----------------
I prefer it was it was


================
Comment at: lib/Target/WebAssembly/CMakeLists.txt:48
   WebAssemblyRuntimeLibcallSignatures.cpp
-  WebAssemblySelectionDAGInfo.cpp
   WebAssemblySetP2AlignOperands.cpp
----------------
What is this?


================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyAsmBackend.cpp:38
       : MCAsmBackend(support::little), Is64Bit(Is64Bit) {}
-  ~WebAssemblyAsmBackend() override {}
+  ~WebAssemblyAsmBackend() override = default;
 
----------------
Should we just remove this line?


================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCAsmInfo.h:26
   explicit WebAssemblyMCAsmInfo(const Triple &T);
-  ~WebAssemblyMCAsmInfo() override;
+  ~WebAssemblyMCAsmInfo() override = default;
 };
----------------
Remove?


================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:246
 void WebAssemblyAsmPrinter::EmitInstruction(const MachineInstr *MI) {
-  LLVM_DEBUG(dbgs() << "EmitInstruction: " << *MI << '\n');
+  LLVM_DEBUG(dbgs() << "emitInstruction: " << *MI << '\n');
 
----------------
Revert.


================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.h:61
   void EmitEndOfAsmFile(Module &M) override;
-  void EmitProducerInfo(Module &M);
+  void emitProducerInfo(Module &M);
   void EmitJumpTableInfo() override;
----------------
Revert this.. presumably its better to match the others.


================
Comment at: lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp:218
   // Start assigning local numbers after the last parameter.
-  unsigned CurLocal = static_cast<unsigned>(MFI.getParams().size());
+  auto CurLocal = static_cast<unsigned>(MFI.getParams().size());
 
----------------
It it worth it in this case?


================
Comment at: lib/Target/WebAssembly/WebAssemblyFastISel.cpp:97
+      Offset = NewOffset;
     }
     int64_t getOffset() const { return Offset; }
----------------
Is this actually better?  Perhaps revert this?


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:326
+  MachineBasicBlock *FalseMBB = F->CreateMachineBasicBlock(LLVMBB);
+  MachineBasicBlock *DoneMBB = F->CreateMachineBasicBlock(LLVMBB);
 
----------------
Revert?  This seem unrelated.


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.h:104
+  SDValue lowerAccessVectorElement(SDValue Op, SelectionDAG &DAG) const;
+  SDValue lowerShift(SDValue Op, SelectionDAG &DAG) const;
 };
----------------
Perhaps lets stick with the ISel convention of calling everything "LowerXX" for now.

Better to match the convention than be locally style conformant I think


================
Comment at: lib/Target/WebAssembly/WebAssemblyMCInstLower.h:34
+  MCSymbol *getGlobalAddressSymbol(const MachineOperand &MO) const;
+  MCSymbol *getExternalSymbolSymbol(const MachineOperand &MO) const;
+  MCOperand lowerSymbolOperand(MCSymbol *Sym, int64_t Offset, bool IsFunc,
----------------
I think this mirror methods in several other classes (e.g. AsmPrinter) so perhaps should keep their names for now


================
Comment at: lib/Target/WebAssembly/WebAssemblySelectionDAGInfo.h:24
 public:
-  ~WebAssemblySelectionDAGInfo() override;
+  ~WebAssemblySelectionDAGInfo() override = default;
 };
----------------
Remove? 


================
Comment at: lib/Target/WebAssembly/WebAssemblyTargetMachine.h:34
 
-  ~WebAssemblyTargetMachine() override;
+  ~WebAssemblyTargetMachine() override = default;
   const WebAssemblySubtarget *
----------------
Remove?


================
Comment at: tools/obj2yaml/wasm2yaml.cpp:373
 
-  return std::error_code();
+  return {};
 }
----------------
revert


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57500