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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: lib/MC/MCSectionWasm.cpp:17
 
-MCSectionWasm::~MCSectionWasm() {} // anchor.
-
----------------
sbc100 wrote:
> I guess you are removing this anchor because we have other methods below?   
Its declaration is marked as `= default` in `include/llvm/MC/MCSectionWasm.h`, like
```
~MCSectionWasm() = default; 
```


================
Comment at: lib/MC/WasmObjectWriter.cpp:1318
 
-        if (WS.getSize() == 0)
+        if (WS.getSize() == nullptr)
           report_fatal_error(
----------------
sbc100 wrote:
> This looks wrong.. unless size is some kind of struct?
It [[ https://github.com/llvm/llvm-project/blob/9e671831212fcdd4f55661f5b369e245c442fb56/llvm/include/llvm/MC/MCSymbolWasm.h#L37 | returns ]] `MCExpr *`.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:83
 
-  ~WebAssemblyOperand() {
+  ~WebAssemblyOperand() override {
     if (isBrList())
----------------
sbc100 wrote:
> Can you explain this change?
The warning was generated by [[ https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html | modernize-use-override ]] rule. But looks like there are debates on whether to do this on destructors and [[ https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c128-virtual-functions-should-specify-exactly-one-of-virtual-override-or-final | CPP core guidelines ]] seems to explicitly discourage it. Reverted this change.


================
Comment at: lib/Target/WebAssembly/CMakeLists.txt:48
   WebAssemblyRuntimeLibcallSignatures.cpp
-  WebAssemblySelectionDAGInfo.cpp
   WebAssemblySetP2AlignOperands.cpp
----------------
tlively wrote:
> sbc100 wrote:
> > What is this?
> I actually put something useful in this file in https://reviews.llvm.org/D57498, so please revert this particular change.
That file contained only a blank destructor
```
WebAssemblySelectionDAGInfo::~WebAssemblySelectionDAGInfo() {}
```
which I marked with `= default` in the header file. But because @tlively is using the file, I'll restore it. 


================
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());
 
----------------
sbc100 wrote:
> It it worth it in this case?
Reverted it.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFastISel.cpp:97
+      Offset = NewOffset;
     }
     int64_t getOffset() const { return Offset; }
----------------
tlively wrote:
> sbc100 wrote:
> > Is this actually better?  Perhaps revert this?
> FWIW, I'm not a fan to adding underscores to create new identifiers.
I'm not either. And also there are a lot of `set***` style functions in LLVM codebase and not many functions are using this style argument name with an underscore. But I don't feel strongly about either option, so please let me know if you still prefer the original code.


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:326
+  MachineBasicBlock *FalseMBB = F->CreateMachineBasicBlock(LLVMBB);
+  MachineBasicBlock *DoneMBB = F->CreateMachineBasicBlock(LLVMBB);
 
----------------
tlively wrote:
> sbc100 wrote:
> > Revert?  This seem unrelated.
> I think clang tidy just doesn't like identifiers that are all caps with underscores.
clang-tidy does not have problems with all caps but does not like underscores. Still revert?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57500